Closed Bug 1328830 Opened 7 years ago Closed 7 years ago

Need a way to set preference values in mochitest.ini and run test in window with the preference values

Categories

(Testing :: Mochitest, defect)

Version 3
defect
Not set
normal

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: hiro, Assigned: ahal)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [PI:July])

Attachments

(3 files, 1 obsolete file)

For example, window.requestIdleCallback can not be used even after enabling its pref in the same window, we need to open a new window after enabling the pref.  If we can specify preference values in mochitest.ini, and then open a new window with the pref and run tests in the window, it will be very useful.
To add another use case, I've made a PI request about this recently:

I have written a test that checks that all images the Firefox frontend loads at startup are actually shown in bug 1363059.

The test file is browser/base/content/test/performance/browser_startup_images.js.

The problem I have is that different images are shown for hidpi screens, but our build infrastructure does not support hidpi in any useful way AFAIK. That means this test currently has undefined behavior on hidpi machines, since we're not consistently testing it on try.

I know there is the layout.css.devPixelsPerPx pref which can be used to simulate a pixel density of 2, which should be what I want. I need a way to set this preference before browser startup in Mochitests.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
In order to land this we need to switch from --run-by-dir to --run-by-manifest. This is because the former, may end up splitting tests from the same manifest into two separate firefox invocations, making it impossible to only set the pref for the proper tests. In theory, the set of tests in a manifest vs in a directory should be nearly the same, so making this change should have minimal impact.

Here is a try push with this change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcba5bfdd7245bcc4992ff3d2bd63b1b0d66441e&group_state=expanded

It looks mostly good except for Windows 10 x64 debug. I'm worried enough about that that I think I'll spin this change out into a new bug and make it block.

Otherwise, I have the prefs in a manifest change fully implemented.
Jmaher pointed out that Windows 10 x64 debug mochitests are all hidden on mozilla-central, those are known failures (which should also be hidden on try).

So looks like this is all good pending the OSX results! I'll upload the patches shortly.
Whiteboard: [PI:July]
Comment on attachment 8888513 [details]
Bug 1328830 - [manifestparser] Check line continuation before looking for next key,

https://reviewboard.mozilla.org/r/159488/#review164854

I like how simple this is and the tests.  what if you had comments:
foo =
  bar # old school
  baz # new school
  
that shouldn't work, but what happens.

Likewise, if we had:
skip-if = os == 'linux' &&
          coverage
          
will this work as expected, or is this invalid?

My questions will be great for a followup since what you have solve the use case you described.
Attachment #8888513 - Flags: review?(jmaher) → review+
Blocks: 1382841
Comment on attachment 8888516 [details]
Bug 1328830 - Add ability to set prefs from the DEFAULT section of a mochitest manifest,

https://reviewboard.mozilla.org/r/159494/#review164858

I am not clear where you split the prefs value on \n and actually set them.

::: testing/mochitest/tests/python/test_get_active_tests.py:57
(Diff revision 1)
> +def test_prefs_validation(get_active_tests, create_manifest):
> +    manifest_relpath, manifest = create_manifest("""
> +[DEFAULT]
> +prefs=
> +  foo=bar
> +  browser.dom.foo=baz

can we put different types of values here, specifically:
 = <int>
 = <bool>
 = <string>
Attachment #8888516 - Flags: review?(jmaher) → review-
Comment on attachment 8888513 [details]
Bug 1328830 - [manifestparser] Check line continuation before looking for next key,

https://reviewboard.mozilla.org/r/159488/#review164854

Line continuations have always existed, they just didn't have a test before. This patch only makes it so they can have an '=' sign in them (needed for prefs, e.g: browser.random.pref=true). In both your examples I don't think my patch should change how anything gets handled.

In the first case, the comments should get stripped out and in the second case, the `skip-if` value would be `os == 'linux' &&\ncoverage`. I *think* that newline will break the expression parser, so afaik, we can't use continuations for `skip-if`. But that's definitely something we could fix if we wanted.
Weird, the OSX results came in from that try push and there are a bunch of failures due to:
FATAL ERROR: Non-local network connections are disabled and a connection attempt to example (52.34.31.151) was made

I have no idea how my changes could cause that.. I wonder if pushed on top of bustage or something.
Those OSX errors are showing up across all trees (even esr-52 somehow). So good news, unrelated to me.. Bad news, wtf is going on?
Comment on attachment 8888516 [details]
Bug 1328830 - Add ability to set prefs from the DEFAULT section of a mochitest manifest,

https://reviewboard.mozilla.org/r/159494/#review164858

> can we put different types of values here, specifically:
>  = <int>
>  = <bool>
>  = <string>

Yep, this mechanism is piggy-backing off the --setpref command line option (options.extraPrefs), which gets forwarded on to mozprofile. Mozprofile converts the string specified by the command line (or now manifest) to the proper type:
https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprofile/mozprofile/prefs.py#54

The prefs get split + appended to options.extraPrefs around line 2360 of this patch.
Comment on attachment 8888516 [details]
Bug 1328830 - Add ability to set prefs from the DEFAULT section of a mochitest manifest,

https://reviewboard.mozilla.org/r/159494/#review165224

thanks for addressing my questions
Attachment #8888516 - Flags: review- → review+
Comment on attachment 8888514 [details]
Bug 1328830 - Forward manifest defaults from the build backend to mochitest when running with mach,

https://reviewboard.mozilla.org/r/159490/#review165834

::: testing/mochitest/mach_commands.py:149
(Diff revision 1)
>  
>          from manifestparser import TestManifest
>          if tests and not options.manifestFile:
>              manifest = TestManifest()
>              manifest.tests.extend(tests)
> +            manifest.manifest_defaults = self.resolver.resolve_manifest_defaults(tests)

I'm not sure I understand this patch exactly, but it looks like we need this to be able to determine from testing/mochitest/runtests.py whether prefs set for a test came from a manifest's default section, and error out if they didn't.

The all-tests.pkl database doesn't really want to implement details of individual manifest formats, we're only taking advantage of the defaults section in manifestparser manifests as an optimization. Would it be possible to disallow prefs set somewhere other the defaults section in the manifest parser instead?
Attachment #8888514 - Flags: review?(cmanchester)
Comment on attachment 8888514 [details]
Bug 1328830 - Forward manifest defaults from the build backend to mochitest when running with mach,

https://reviewboard.mozilla.org/r/159490/#review165834

> I'm not sure I understand this patch exactly, but it looks like we need this to be able to determine from testing/mochitest/runtests.py whether prefs set for a test came from a manifest's default section, and error out if they didn't.
> 
> The all-tests.pkl database doesn't really want to implement details of individual manifest formats, we're only taking advantage of the defaults section in manifestparser manifests as an optimization. Would it be possible to disallow prefs set somewhere other the defaults section in the manifest parser instead?

Yes you have the use case correct.

It would be possible to check this in manifestparser, but not desirable as this is a mochitest-only manifest key. There's both an all-tests.pkl database as well as a test-defaults.pkl database. As I see it, this patch is adding a way to query the latter.
Comment on attachment 8888514 [details]
Bug 1328830 - Forward manifest defaults from the build backend to mochitest when running with mach,

https://reviewboard.mozilla.org/r/159490/#review165834

> Yes you have the use case correct.
> 
> It would be possible to check this in manifestparser, but not desirable as this is a mochitest-only manifest key. There's both an all-tests.pkl database as well as a test-defaults.pkl database. As I see it, this patch is adding a way to query the latter.

Would you mind clarifying your concern here? Is it the added API surface on TestResolver? If you prefer, I'd be happy simply with the self.default change on the TestMetadata class and I can perform the default lookup directly in testing/mochitest/mach_commands.py.
Flags: needinfo?(cmanchester)
Comment on attachment 8888515 [details]
Bug 1328830 - Switch mochitest's --run-by-dir feature to --run-by-manifest,

https://reviewboard.mozilla.org/r/159492/#review166304

This looks good, but I'd like to make sure the logPreamble() issue gets sorted out before proceeding.

::: testing/mochitest/mochitest_options.py:235
(Diff revision 1)
>            "default": 0,
>            }],
> -        [["--run-by-dir"],
> +        [["--run-by-manifest"],
>           {"action": "store_true",
> -          "dest": "runByDir",
> +          "dest": "runByManifest",
>            "help": "Run each directory in a single browser instance with a fresh profile.",

Let's update the help while we're here. At least s/directory/manifest/.

::: testing/mochitest/runtests.py:2328
(Diff revision 1)
>          self.killNamedOrphans('xpcshell')
>  
>          if options.cleanupCrashes:
>              mozcrash.cleanup_pending_crash_reports()
>  
>          # Until we have all green, this only runs on bc*/dt*/mochitest-chrome

This comment seems a little out of place. Should it maybe be immediately above 'if not options.runByManifest'?

::: testing/mochitest/runtests.py:2332
(Diff revision 1)
>  
>          # Until we have all green, this only runs on bc*/dt*/mochitest-chrome
>          # jobs, not jetpack*, a11yr (for perf reasons), or plain
>  
> -        testsToRun = self.getTestsToRun(options)
> -        if not options.runByDir:
> +        tests = [t for t in self.getActiveTests(options) if 'disabled' not in t]
> +        self.logPreamble(tests)

By calling logPreamble() after filtering, you won't log anything for disabled tests, I think. Was that intended? It seems wrong.
Attachment #8888515 - Flags: review?(gbrown)
(In reply to Andrew Halberstadt [:ahal] from comment #17)
> Comment on attachment 8888514 [details]
> Bug 1328830 - Forward manifest defaults from the build backend to mochitest
> when running with mach,
> 
> https://reviewboard.mozilla.org/r/159490/#review165834
> 
> > Yes you have the use case correct.
> > 
> > It would be possible to check this in manifestparser, but not desirable as this is a mochitest-only manifest key. There's both an all-tests.pkl database as well as a test-defaults.pkl database. As I see it, this patch is adding a way to query the latter.
> 
> Would you mind clarifying your concern here? Is it the added API surface on
> TestResolver? If you prefer, I'd be happy simply with the self.default
> change on the TestMetadata class and I can perform the default lookup
> directly in testing/mochitest/mach_commands.py.

I don't think the TestResolver or anything consuming all-tests.pkl should be implementing details of test manifest formats, one of their main purposes is to pave over those differences. I guess it's not a perfect fit because this is mochitest-only, but unless we end up adding a prefs section to other harnesses with different semantics I don't think it's that unfortunate to implement this validation in the manifest parser. After all it's the manifest that's malformed if someone ends up misusing the prefs section.
Flags: needinfo?(cmanchester)
It's not implementing manifest specific details though. The build system abstracts the concept of "manifest-defaults", and it's consuming that.

From my point of view: some manifest formats have a DEFAULT concept (of which manifestparser is but one), and the build system tramples all over that concept and fails to provide any mechanism to retrieve that information. This patch fixes that.
I guess I can read manifest_defaults.pkl directly from testing/mochitest/mach_commands.py if needed, but that seems a shame as it's already in memory.
:ahal, are you planning to finish this up, or should we put it on the backburner?
Flags: needinfo?(ahalberstadt)
Yes I was planning to finish this up this week, but got a bit distracted by some asan failures. I'll get a new patch up today.
Flags: needinfo?(ahalberstadt)
Comment on attachment 8888515 [details]
Bug 1328830 - Switch mochitest's --run-by-dir feature to --run-by-manifest,

https://reviewboard.mozilla.org/r/159492/#review166304

> By calling logPreamble() after filtering, you won't log anything for disabled tests, I think. Was that intended? It seems wrong.

Good catch!
Comment on attachment 8888514 [details]
Bug 1328830 - Forward manifest defaults from the build backend to mochitest when running with mach,

https://reviewboard.mozilla.org/r/159490/#review165834

> Would you mind clarifying your concern here? Is it the added API surface on TestResolver? If you prefer, I'd be happy simply with the self.default change on the TestMetadata class and I can perform the default lookup directly in testing/mochitest/mach_commands.py.

Hopefully this new patch is a good compromise. It's still using manifest-defaults.pkl, but at least there's no added API surface to the build system.

I kind of get what you're saying, but between implementing this by reading manifest-defaults.pkl and hardcoding a check into manifestparser, using manifest-defaults.pkl seems to be by far the lesser of two evils.
Comment on attachment 8888515 [details]
Bug 1328830 - Switch mochitest's --run-by-dir feature to --run-by-manifest,

https://reviewboard.mozilla.org/r/159492/#review169890
Attachment #8888515 - Flags: review?(gbrown) → review+
Comment on attachment 8888514 [details]
Bug 1328830 - Forward manifest defaults from the build backend to mochitest when running with mach,

https://reviewboard.mozilla.org/r/159490/#review170412

This doesn't really address my concerns here. As a concrete example of why this concerns me, until recently we didn't store manifest defaults in a separate file at all, but this patch would commit us to continuing to do that in the future. It seems like bad coupling.
Attachment #8888514 - Flags: review?(cmanchester)
Do you have any ideas how I can solve this then (keeping in mind hardcoding the pref in manifestparser is also bad coupling)? The reason I was hoping to fix this in the build system is because the build system doesn't expose manifest defaults anywhere, so that information gets lost when we use the build system test resolving. To me that's a bug.

Maybe this isn't the best way of solving that bug, but in that case, please point me in the right direction.
Flags: needinfo?(cmanchester)
As I've said twice before, it seems preferable to validate the manifests while parsing.
Flags: needinfo?(cmanchester)
I'm not really sure where you mean, somewhere around here?
https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/emitter.py#1260

But I just thought of an alternative way.. instead of asserting that the prefs are in DEFAULT, I could assert that every test in a manifest has the same set of prefs. It's a bit of extra work, but can be done entirely from mochitest. I think I'll just do that.
Attachment #8888514 - Attachment is obsolete: true
Comment on attachment 8888516 [details]
Bug 1328830 - Add ability to set prefs from the DEFAULT section of a mochitest manifest,

Hey Joel, this got changed enough I think I'd like a re-review. You can see the interdiff since the last time you reviewed here:
https://reviewboard.mozilla.org/r/159494/diff/2-3/
Attachment #8888516 - Flags: review+ → review?(jmaher)
Comment on attachment 8888516 [details]
Bug 1328830 - Add ability to set prefs from the DEFAULT section of a mochitest manifest,

https://reviewboard.mozilla.org/r/159494/#review171246

thanks for the update, I was confused at first, then realized that you got some other changes mixed in here from a rebase.
Attachment #8888516 - Flags: review?(jmaher) → review+
Latest series looking good on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a8a6704692b1d1762884265e9dd5d0f4029ff64

(though I didn't run on osx to save capacity, so murphy's law says I'll be backed out for osx bustage)
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d64da85644e
[manifestparser] Check line continuation before looking for next key, r=jmaher
https://hg.mozilla.org/integration/autoland/rev/fbf29cfa4717
Switch mochitest's --run-by-dir feature to --run-by-manifest, r=gbrown
https://hg.mozilla.org/integration/autoland/rev/687cced20fd6
Add ability to set prefs from the DEFAULT section of a mochitest manifest, r=jmaher
https://hg.mozilla.org/mozilla-central/rev/8d64da85644e
https://hg.mozilla.org/mozilla-central/rev/fbf29cfa4717
https://hg.mozilla.org/mozilla-central/rev/687cced20fd6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Awesome! Thank you :ahal!
Depends on: 1389500
Blocks: 1395305
Just to let you all know — we are no longer contributing to non-web platform docs on the MDN team. We are hopefully getting someone in soon to handle internals docs, which this might fall under, but I can't guarantee that yet. I'll make a record of this bug so it doesn't get forgotten about, but I can't guarantee it'll be sorted any time soon.
Thanks for the heads up Chris, and no problem. Ideally I think we want to move these docs into the tree anyway (hosted at firefox-source-docs.mozilla.org). I guess once docs are in-tree we won't need to set a flag anymore, the docs should land as part of the change its documenting.
See Also: → 1529000
See Also: → 1551885
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: