Page MenuHomePhabricator

Unit tests in FormatJsonTest fail
Closed, ResolvedPublic

Description

The unit tests in mediawiki-core-regression-hhvm-master fail (cf. https://integration.wikimedia.org/ci/job/mediawiki-core-regression-hhvm-master/2479/console):

[...]
18:40:04 There were 10 failures:
18:40:04
18:40:04 1) FormatJsonTest::testParseTryFixing with data set #3 ('[1],', false)
18:40:04 Failed asserting that true is false.
18:40:04
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/includes/json/FormatJsonTest.php:199
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/MediaWikiTestCase.php:141
18:40:04
18:40:04 2) FormatJsonTest::testParseTryFixing with data set #4 ('[1,]', '[1]')
18:40:04 Failed asserting that true is false.
18:40:04
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/includes/json/FormatJsonTest.php:201
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/MediaWikiTestCase.php:141
18:40:04
18:40:04 3) FormatJsonTest::testParseTryFixing with data set #5 ('[1
18:40:04 ,]', '[1]')
18:40:04 Failed asserting that true is false.
18:40:04
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/includes/json/FormatJsonTest.php:201
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/MediaWikiTestCase.php:141
18:40:04
18:40:04 4) FormatJsonTest::testParseTryFixing with data set #6 ('[1,
18:40:04 ]', '[1]')
18:40:04 Failed asserting that true is false.
18:40:04
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/includes/json/FormatJsonTest.php:201
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/MediaWikiTestCase.php:141
18:40:04
18:40:04 5) FormatJsonTest::testParseTryFixing with data set #7 ('[1,]
18:40:04 ', '[1]')
18:40:04 Failed asserting that true is false.
18:40:04
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/includes/json/FormatJsonTest.php:201
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/MediaWikiTestCase.php:141
18:40:04
18:40:04 6) FormatJsonTest::testParseTryFixing with data set #8 ('[1
18:40:04 ,
18:40:04 ]
18:40:04 ', '[1]')
18:40:04 Failed asserting that true is false.
18:40:04
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/includes/json/FormatJsonTest.php:201
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/MediaWikiTestCase.php:141
18:40:04
18:40:04 7) FormatJsonTest::testParseTryFixing with data set #9 ('["a,",]', '["a,"]')
18:40:04 Failed asserting that true is false.
18:40:04
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/includes/json/FormatJsonTest.php:201
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/MediaWikiTestCase.php:141
18:40:04
18:40:04 8) FormatJsonTest::testParseTryFixing with data set #10 ('
18:40:04 ,[2,
18:40:04 ],[3
18:40:04 ,]]', '[[1],[2],[3]]')
18:40:04 Failed asserting that true is false.
18:40:04
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/includes/json/FormatJsonTest.php:201
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/MediaWikiTestCase.php:141
18:40:04
18:40:04 9) FormatJsonTest::testParseTryFixing with data set #11 ('[[1,],[2,],[3,]]', false)
18:40:04 Failed asserting that true is false.
18:40:04
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/includes/json/FormatJsonTest.php:199
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/MediaWikiTestCase.php:141
18:40:04
18:40:04 10) FormatJsonTest::testParseTryFixing with data set #12 ('[1,,]', false)
18:40:04 Failed asserting that true is false.
18:40:04
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/includes/json/FormatJsonTest.php:199
18:40:04 /mnt/jenkins-workspace/workspace/mediawiki-core-regression-hhvm-master/src/tests/phpunit/MediaWikiTestCase.php:141
18:40:04
[...]

(I am confused by the Jenkins UI at the moment. At https://integration.wikimedia.org/ci/job/mediawiki-core-regression-hhvm-master/, I'm not sure if "Last successful build" refers to the /chronological/ order in which the tests are run, or if that reflects the Git ancestry (that being useful, the former not).)


Version: 1.25-git
Severity: normal

Details

Reference
bz72778

Event Timeline

bzimport raised the priority of this task from to Normal.Nov 22 2014, 3:50 AM
bzimport set Reference to bz72778.
scfc created this task.Oct 30 2014, 8:05 PM

HHVM has a lenient json parser which allows trailing commas. The failing tests seem to be ones where the test expects json_decode() to fail with JSON_ERROR_SYNTAX and then need to have extra commas stripped.

Block that ignores trailing commas: https://github.com/facebook/hhvm/blob/2d64184dd67068e02d3069bbc045bca031498465/hphp/runtime/ext/json/JSON_parser.cpp#L707-L720

$ /usr/bin/php5 -r 'var_dump( json_decode("[0,]") );'
NULL

$ /usr/bin/hhvm --php -r 'var_dump( json_decode("[0,]") );'
array(1) {

[0]=>
int(0)

}

I'm really not sure why it is always enabled. It looks like there is an option flag to enable/disable the behavior.

gerritadmin wrote:

Change 170251 had a related patch set uploaded by BryanDavis:
hhvm: fix FormatJsonTest::testParseTryFixing for lenient json parser

https://gerrit.wikimedia.org/r/170251

gerritadmin wrote:

Change 170251 merged by jenkins-bot:
hhvm: fix FormatJsonTest::testParseTryFixing for lenient json parser

https://gerrit.wikimedia.org/r/170251

bd808 added a comment.Oct 31 2014, 6:12 PM

Passing now. https://integration.wikimedia.org/ci/job/mediawiki-core-regression-hhvm-master/2495/console

00:05:49.197 OK, but incomplete or skipped tests!
00:05:49.201 Tests: 9141, Assertions: 1600131, Skipped: 18.

scfc added a comment.Nov 2 2014, 8:35 PM

Interestingly, while this has silenced the Jenkins tests, the change has introduced failures in the same spots with Travis CI's HHVM: The commit before passed (cf. https://travis-ci.org/wikimedia/mediawiki/jobs/39621924), this one (cf. https://travis-ci.org/wikimedia/mediawiki/jobs/39624363) and the ones after fail now with:

[...]
2) FormatJsonTest::testParseTryFixing with data set #3 ('[1],', false, true, '[1]')
Expected isGood() == true
Failed asserting that false matches expected true.
/home/travis/build/wikimedia/mediawiki/tests/phpunit/includes/json/FormatJsonTest.php:236
/home/travis/build/wikimedia/mediawiki/tests/phpunit/MediaWikiTestCase.php:141
3) FormatJsonTest::testParseTryFixing with data set #4 ('[1,]', '[1]')
Expected isGood() == true
Failed asserting that false matches expected true.
/home/travis/build/wikimedia/mediawiki/tests/phpunit/includes/json/FormatJsonTest.php:236
/home/travis/build/wikimedia/mediawiki/tests/phpunit/MediaWikiTestCase.php:141
4) FormatJsonTest::testParseTryFixing with data set #5 ('[1
,]', '[1]')
Expected isGood() == true
Failed asserting that false matches expected true.
/home/travis/build/wikimedia/mediawiki/tests/phpunit/includes/json/FormatJsonTest.php:236
/home/travis/build/wikimedia/mediawiki/tests/phpunit/MediaWikiTestCase.php:141
5) FormatJsonTest::testParseTryFixing with data set #6 ('[1,
]', '[1]')
Expected isGood() == true
Failed asserting that false matches expected true.
/home/travis/build/wikimedia/mediawiki/tests/phpunit/includes/json/FormatJsonTest.php:236
/home/travis/build/wikimedia/mediawiki/tests/phpunit/MediaWikiTestCase.php:141
6) FormatJsonTest::testParseTryFixing with data set #7 ('[1,]
', '[1]')
Expected isGood() == true
Failed asserting that false matches expected true.
/home/travis/build/wikimedia/mediawiki/tests/phpunit/includes/json/FormatJsonTest.php:236
/home/travis/build/wikimedia/mediawiki/tests/phpunit/MediaWikiTestCase.php:141
7) FormatJsonTest::testParseTryFixing with data set #8 ('[1
,
]
', '[1]')
Expected isGood() == true
Failed asserting that false matches expected true.
/home/travis/build/wikimedia/mediawiki/tests/phpunit/includes/json/FormatJsonTest.php:236
/home/travis/build/wikimedia/mediawiki/tests/phpunit/MediaWikiTestCase.php:141
8) FormatJsonTest::testParseTryFixing with data set #9 ('["a,",]', '["a,"]')
Expected isGood() == true
Failed asserting that false matches expected true.
/home/travis/build/wikimedia/mediawiki/tests/phpunit/includes/json/FormatJsonTest.php:236
/home/travis/build/wikimedia/mediawiki/tests/phpunit/MediaWikiTestCase.php:141
9) FormatJsonTest::testParseTryFixing with data set #10 ('
,[2,
],[3
,]]', '[[1],[2],[3]]')
Expected isGood() == true
Failed asserting that false matches expected true.
/home/travis/build/wikimedia/mediawiki/tests/phpunit/includes/json/FormatJsonTest.php:236
/home/travis/build/wikimedia/mediawiki/tests/phpunit/MediaWikiTestCase.php:141
10) FormatJsonTest::testParseTryFixing with data set #11 ('[[1,],[2,],[3,]]', false, true, '[[1],[2],[3]]')
Expected isGood() == true
Failed asserting that false matches expected true.
/home/travis/build/wikimedia/mediawiki/tests/phpunit/includes/json/FormatJsonTest.php:236
/home/travis/build/wikimedia/mediawiki/tests/phpunit/MediaWikiTestCase.php:141
11) FormatJsonTest::testParseTryFixing with data set #12 ('[1,,]', false, false, '[1]')
Expected isOK == true
Failed asserting that false is true.
/home/travis/build/wikimedia/mediawiki/tests/phpunit/includes/json/FormatJsonTest.php:237
/home/travis/build/wikimedia/mediawiki/tests/phpunit/MediaWikiTestCase.php:141
[...]

Panta rhei.

bd808 added a comment.Nov 3 2014, 12:57 AM

(In reply to Tim Landscheidt from comment #6)

Interestingly, while this has silenced the Jenkins tests, the change has
introduced failures in the same spots with Travis CI's HHVM: The commit
before passed (cf. https://travis-ci.org/wikimedia/mediawiki/jobs/39621924),
this one (cf. https://travis-ci.org/wikimedia/mediawiki/jobs/39624363) and
the ones after fail now with:

[...]
2) FormatJsonTest::testParseTryFixing with data set #3 ('[1],', false,

true, '[1]')

Expected isGood() == true
Failed asserting that false matches expected true.

I wonder if the HHVM packages that the WMF is building have some Facebook default settings enabled that aren't enabled in the builds used by Travis CI? When I looked through the upstream source for HHVM it looked to me like the JSON loose parsing behavior should not have been enabled by default and instead would require a custom HHVM option (JSON_FB_LOOSE) to be passed to json_decode to enable the loose parse behavior.

I'll look a bit deeper and see if I can figure this out. I think that if we are enabling this in our build it is probably by accident. If nothing else, I can probably craft a guard to add to the test that detects if the default behavior is loose or not.

bd808 added a comment.Nov 4 2014, 1:27 AM

Ori pointed out that the CMake rules will use a shared libjson-c library if present and the WMF HHVM build is using that:

$ ldd /usr/bin/hhvm |grep json

libjson-c.so.2 => /lib/x86_64-linux-gnu/libjson-c.so.2 (0x00007ff80354e000)

I haven't investigated yet if this would change json_decode() to be lenient by default, but it is a possibility.

bd808 added a comment.Nov 8 2014, 9:58 PM

(In reply to Bryan Davis from comment #8)

Ori pointed out that the CMake rules will use a shared libjson-c library if
present and the WMF HHVM build is using that:
$ ldd /usr/bin/hhvm |grep json

libjson-c.so.2 => /lib/x86_64-linux-gnu/libjson-c.so.2

(0x00007ff80354e000)
I haven't investigated yet if this would change json_decode() to be lenient
by default, but it is a possibility.

Libjson-c is lenient by default https://github.com/json-c/json-c/blob/d4e81f9ec8273914739808737fa0a27a3f0589fb/json_tokener.h#L90-L100 and this is not changed by HHVM https://github.com/facebook/hhvm/blob/a82234b63e86d57026db719e533225f25f103127/hphp/runtime/ext/json/jsonc_parser.cpp#L225-L227. This combination means that the behavior of json_decode() changes depending on how HHVM is compiled/linked.

gerritadmin wrote:

Change 172090 had a related patch set uploaded by BryanDavis:
hhvm: Detect json-c parser

https://gerrit.wikimedia.org/r/172090

gerritadmin wrote:

Change 172090 merged by jenkins-bot:
hhvm: Detect json-c parser

https://gerrit.wikimedia.org/r/172090