phpunit hhvm failure: LuaSandbox: TextLibraryTests[87]: json decode, invalid values (trailing comma)
Closed, DuplicatePublic
Actions

Assigned To

Authored By

	JanZerebecki
	Jun 22 2015, 2:08 PM

Description

This is an issue with HHVM using json-c's default non-strict parsing mode. Upstream bug is https://github.com/facebook/hhvm/issues/5813

I tried running the CI for the build of Wikidata with hhvm instead of zend:
https://integration.wikimedia.org/ci/job/mwext-Wikidata-testextension-hhvm/2/consoleFull
Works on zend: https://integration.wikimedia.org/ci/job/mwext-Wikidata-testextension-zend/359/

There were 2 failures:

LuaSandbox: TextLibraryTests[87]: json decode, invalid values (trailing comma)

Failed asserting that two strings are identical.

Expected

+++ Actual
@@ @@
-ERROR: mw.text.jsonDecode: Syntax error
+{
+ {
+ ["x"] = 1,
+ },
+}

/mnt/jenkins-workspace/workspace/mwext-Wikidata-testextension-hhvm/src/extensions/Scribunto/tests/engines/LuaCommon/LuaEngineTestBase.php:252
/mnt/jenkins-workspace/workspace/mwext-Wikidata-testextension-hhvm/src/tests/phpunit/MediaWikiTestCase.php:131

LuaStandalone: TextLibraryTests[87]: json decode, invalid values (trailing comma)

Failed asserting that two strings are identical.

Expected

+++ Actual
@@ @@
-ERROR: mw.text.jsonDecode: Syntax error
+{
+ {
+ ["x"] = 1,
+ },
+}

Details

Subject	Repo	Branch	Lines +/-
Temporarily skip a test that fails on HHVM.	mediawiki/extensions/Scribunto	REL1_26	+9 -0
Temporarily skip a test that fails on HHVM.	mediawiki/extensions/Scribunto	master	+9 -0
Temporarily skip a test that fails on HHVM.	mediawiki/extensions/Scribunto	REL1_25	+9 -0

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Resolved	hashar	T60772 common gating job for mediawiki core and extensions
Resolved	hashar	T69216 Have unit tests of all wmf deployed extensions pass when installed together, in both PHP-Zend and HHVM (tracking)
Resolved	None	T75521 Jenkins: Implement hhvm based voting jobs for mediawiki and extensions
Invalid	None	T112073 Lua in Wikibase (tracking)
Duplicate	• Mattflaschen-WMF	T103346 phpunit hhvm failure: LuaSandbox: TextLibraryTests[87]: json decode, invalid values (trailing comma)

Event Timeline

JanZerebecki created this task.Jun 22 2015, 2:08 PM

JanZerebecki raised the priority of this task from to Needs Triage.

JanZerebecki updated the task description. (Show Details)

JanZerebecki added projects: Scribunto, Wikidata.

JanZerebecki moved this task to Backlog on the Scribunto board.

JanZerebecki subscribed.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 22 2015, 2:08 PM

JanZerebecki added a parent task: T75521: Jenkins: Implement hhvm based voting jobs for mediawiki and extensions.Jun 22 2015, 2:10 PM

Bug in HHVM: the json-c library used by hhvm accepts input that's not
actually valid json by default, and HHVM doesn't pass the override flag or
even expose it to PHP code.

Anomie added a project: HHVM.Jun 30 2015, 8:56 PM

Anomie set Security to None.

Anomie updated the task description. (Show Details)Jul 28 2015, 3:09 PM

Anomie added a project: Upstream.

What's the actual problem here? Are our tests failing, because input that shouldn't be accepted is accepted? Or what?
Do we actually need to fail on a trailing comma? Is it part of our contract to perform strict validation on json input?
What JSON input anyway?

daniel moved this task from incoming to needs discussion or investigation on the Wikidata board.Sep 10 2015, 1:20 PM

Our tests fail correctly because hhvm accepts input that is invalid json that it should not accept. Suggested action: Fix hhvm.

If the relevant input isn't actually harmful, I'd suggest to relax the tests.
Sure, hhvm should be fixed, and there's a ticket for that. But *we* shouldn't block on hhvm getting fixed.

daniel moved this task from needs discussion or investigation to monitoring on the Wikidata board.Sep 10 2015, 1:54 PM

In T103346#1625120, @daniel wrote:

Are our tests failing, because input that shouldn't be accepted is accepted?

Yes. For example, [1,] is not valid JSON (it's not allowed in either RFC 7159 or ECMA-404), but is accepted by HHVM's json_decode.

Do we actually need to fail on a trailing comma? Is it part of our contract to perform strict validation on json input?

It should at least be consistent on PHP versus HHVM, which currently it's not as shown by this test failure. In general it's best to follow the standards, IMO.

What JSON input anyway?

Whatever JSON input is supplied by the user.

This tests the json functions we implement for Lua, so yes we need to perform strict validation, as we specified our Lua json_decode function that way.

A compromise would be to mark the relevant test as skipped until this issue is fixed in hhvm.

matej_suchanek added a parent task: T112073: Lua in Wikibase (tracking).Sep 10 2015, 5:20 PM

JanZerebecki mentioned this in T113049: [Task] add CI to extension ArticlePlaceholder.Nov 11 2015, 6:11 PM

Change 252508 had a related patch set uploaded (by JanZerebecki):
Temporarily skip a test that fails on HHVM.

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

Restricted Application added a subscriber: StudiesWorld. · View Herald TranscriptNov 11 2015, 8:49 PM

gerritbot added a project: Patch-For-Review.Nov 11 2015, 8:49 PM

Change 252508 merged by jenkins-bot:
Temporarily skip a test that fails on HHVM.

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

ReleaseTaggerBot added a project: MW-1.27-release (WMF-deploy-2015-11-17_(1.27.0-wmf.7)).Nov 11 2015, 10:00 PM

Anomie mentioned this in T125722: Wikidata broken on branch REL1_25 causing other extensions to fail..Feb 4 2016, 1:40 AM

Change 268473 had a related patch set uploaded (by Mattflaschen):
Temporarily skip a test that fails on HHVM.

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

Change 268473 merged by jenkins-bot:
Temporarily skip a test that fails on HHVM.

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

Change 268811 had a related patch set uploaded (by Paladox):
Temporarily skip a test that fails on HHVM.

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

Preventing us from merging https://gerrit.wikimedia.org/r/#/c/260869/ .

• Mattflaschen-WMF added a project: Collaboration-Team-Archive-2015-2016.Feb 5 2016, 10:26 PM

• Mattflaschen-WMF mentioned this in T126073: Wikidata QUnit broken on branch REL1_25 causing other extensions to fail.Feb 5 2016, 10:41 PM

Change 268811 merged by jenkins-bot:
Temporarily skip a test that fails on HHVM.

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

Catrope closed this task as Resolved.Feb 13 2016, 12:29 AM

Catrope subscribed.

The bug is not fixed, yet. Just the test disabled.

Catrope removed a project: Collaboration-Team-Archive-2015-2016.Feb 19 2016, 11:56 PM

Krinkle moved this task from Backlog to Defect on the HHVM board.Feb 21 2016, 5:57 PM

Jdforrester-WMF mentioned this in T128029: TemplateData's PHP JSON validation isn't strict enough because WMF cluster's HHVM allows trailing commas.Feb 25 2016, 1:13 AM

ori closed this task as a duplicate of T128029: TemplateData's PHP JSON validation isn't strict enough because WMF cluster's HHVM allows trailing commas.Mar 6 2016, 2:08 AM

Anomie mentioned this in T214984: PHP7's stricter JSON parsing breaks some wiki content.Feb 7 2019, 2:28 PM

Aklapper removed subscribers: • Mattflaschen-WMF, Anomie.Oct 16 2020, 5:39 PM

phpunit hhvm failure: LuaSandbox: TextLibraryTests[87]: json decode, invalid values (trailing comma)Closed, DuplicatePublicActions

Description

Details

Related ObjectsSearch...

Event Timeline

phpunit hhvm failure: LuaSandbox: TextLibraryTests[87]: json decode, invalid values (trailing comma)
Closed, DuplicatePublic
Actions

Related Objects
Search...