1228678 - space around the colon in the syntax used for setting prefs is significant

Status: RESOLVED FIXED

(Testing :: web-platform-tests, defect)

Description

[(no longer active)]

If you have the following in an ini file:

prefs: [foo.bar: true]

You will be setting the pref value to |" true"| not to |true| which is what the author probably expects.  For boolean prefs, non-empty string values are interpreted as true, so this does what the author actually expects by accident.

But once you do:

prefs: [foo.bar: false]

we will set the pref to |" false"| which is also interpreted as true.

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Bug 1228678 - Strip out leading/trailing whitespace from wpt pref names and values r?jgraham

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This patch just strips the spaces out from the pref names and values prior to using them to set Firefox prefs.

And a try run to see if that breaks things: https://treeherder.mozilla.org/#/jobs?repo=try&revision=785bcb2b17ebaee6d114c4357ea649e55d7bc037

Are the test metadata files linted anywhere? I know the manifest and the tests themselves have linters...

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Bug 1228678 - Remove leading/trailing whitespace in wpt pref values, also strip whitespace in prefs when used

Also fix in-tree instances of leading/trailing whitespaces in pref values.

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 9040297 [details]
Bug 1228678 - Remove leading/trailing whitespace in wpt pref values, also strip whitespace in prefs when used

This version fixes the in-tree instances of leading/trailing whitespaces in pref values and makes leading/trailing whitespace in pref values fatal to the test run by printing out an error message and stopping execution.

At the moment, encountering a leading/trailing whitespace makes the test run fail like this in Treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&classifiedState=unclassified&group_state=expanded&revision=fb8b3d974ba8cca2f92b95c6a011413a2e1815dc&selectedJob=225098069

And the full failure log includes the message about the messed up preference: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=225098069&repo=try&lineNumber=990

BUT...

1. It doesn't display the proper message as a failure line in TH's main UI. I assume I need to import and set up a logger rather than print()ing to stdout or stderr?

2. It just calls sys.exit(1) to stop the test run after printing out the error message. There must be some way for me to trigger the end of the test run, right? Or should I print out the error message and let the test run continue despite the whitespace?

3. I don't think manifestexpected.py always has a reference to the filename/path of the manifest with the whitespaced prefs. I think it might be that DirectoryManifest manifests don't have the "id" property while TestNode manifests do? Is there some way to get that information for all of the manifests so the error message can be made to point to the offending file?

Just f?you for now to see if this is going in the right direction and to help me resolve 1-3.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5f18fc8b26b
Remove leading/trailing whitespace in wpt pref values, also strip whitespace in prefs when used r=jgraham

Comment 6

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15258 for changes under testing/web-platform/tests

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b5f18fc8b26b

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Bug 1228678 - Followup to fix failing upstream wpt test r?jgraham

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This is to fix an upstream test that started failing in comment 6's PR. In the test, one of the values for prefs is "@Reset" which is apparently of type 'object' and can't be strip()'d. So lets just not attempt to strip() those.

I'm not entirely sure how these things get synced back upstream. Will this new patch just fix things there? Do I need to revert the original patch and submit a combined one?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e80ee365c75a
Followup to fix failing upstream wpt test r=jgraham

Comment 11

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/e80ee365c75a