Closed Bug 1197365 Opened 4 years ago Closed 4 years ago

run wpt tests in e10s in automation


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

Not set


(e10s+, firefox43 fixed)

Tracking Status
e10s + ---
firefox43 --- fixed


(Reporter: bkelly, Assigned: jgraham)


(Blocks 1 open bug)



(5 files, 1 obsolete file)

There is a desire to move more of our main testing out of mochitests and into wpt tests.  This can't really happen, though, unless we run wpt on the same platforms as mochitests.  Getting wpt running on debug was a huge improvement.  The next hurdle seems to get wpt running on e10s in addition to non-e10s.

For example, I'm writing wpt tests for service workers.  Unfortunately, though, we have a fair number of e10s-only bugs due to the way the necko stack is architected differently in that mode.  Without running wpt in e10s we can't rely on the tests to provide coverage.
Jonathan, I'm told this is a problem.  Can you explain why?
Flags: needinfo?(jgriffin)
So I see three options here:

1) We continue as-is and people write tests for features that behave differently under e10s using mochitest (service workers, streams, anything else that touches the network or storage) rather than wpt because they get better coverage.
2) We duplicate all web-platform-tests in e10s enabled and e10s disabled configurations.
3) We duplicate a subset of wpt in an e10s-enabled configuration, according to the features that are most likely to vary between the two configurations (or, conversely, we switch all tests to run under e10s and duplicate a subset under non-e10s).

1) is not an option I like at all; it prevents us sharing our tests with other browser vendors so although it may help e10s ship it won't help us with the wider goal of having great implementation quality / compatibility. 2) is the obvious option, but may be prohibitively expensive in terms of machine time and number of added jobs. 3) is a compromise position that provides less coverage and manual work to identify the tests most likely to differ but will save infrastructure resources.

jgriffin: do you have a feeling for whether 2) is acceptable or whether we have to do 3)?
I would rather wpt tests run in both e10s and non-e10s by default.  If you have some tests you think should not be effected by the processing mode, then specifically opt those into one mode or the other.

Lets not create extra stumbling blocks to writing new wpt tests like requesting full coverage for them.

(After all, if I write them as mochitests they automatically get e10s and non-e10s testing anyway.)
I think 2) is the preferred approach, with the caveat that we may not be able to roll it out completely for Windows, due to capacity limitations.  On Windows, I think we should start with win7-opt, assess the impact, then consider rolling out win7-debug or win8-opt, etc.
Flags: needinfo?(jgriffin) is wpt with all tests running under e10s. Clearly it's going to be necessary to have some different metadata for e10s vs not-e10s. I think the easiest way to do that is to manually add it to the run_info and allow the manifests to use 'e10s' as a boolean variable like 'debug'. The only other sensible option I can think of is to duplicate all the metadata, but that seems unpleasant for obvious reasons.
Attached patch wpt-e10s.diffSplinter Review
Buildbot config changes to enable wpt-e10s on Try only. This depends on the wptrunner/mozharness changes (forthcoming) to add the --e10s option; presumably for builds missing that we'll just get an error. Since this is only Try I'm not that worried; the jobs will initially fail anyway and so should be hidden.
Attachment #8654186 - Flags: review?(jgriffin)
wptrunner bits need to be upstreamed, obviously.

This seems to work OK in some local testing, but the nature of the patch means I can't run it through try.
Attachment #8654202 - Flags: review?(ahalberstadt)
Comment on attachment 8654186 [details] [diff] [review]

Review of attachment 8654186 [details] [diff] [review]:

::: mozilla-tests/
@@ +747,5 @@
> +    ('web-platform-tests-reftests-e10s', {
> +        'use_mozharness': True,
> +        'script_path': 'scripts/',
> +        'extra_args': ["--test-type=reftest"],

Is this missing an --e10s arg?
Attachment #8654186 - Flags: review?(jgriffin) → review+
Also, feel free to continue using cedar for this, we're keeping it around with a reduced job set for the time being.
Comment on attachment 8654202 [details] [diff] [review]

Review of attachment 8654202 [details] [diff] [review]:

Lgtm! (I guess) :)

::: testing/web-platform/harness/wptrunner/
@@ +43,5 @@
>      browser_kwargs = getattr(module, data["browser_kwargs"])
>      executor_kwargs = getattr(module, data["executor_kwargs"])
>      env_options = getattr(module, data["env_options"])()
> +    run_info_extras = (getattr(module, data["run_info_extras"])
> +                       if "run_info_extras" in data else lambda **kwargs:{})

Neat, I've never really thought of **kwargs in a lambda before.
Attachment #8654202 - Flags: review?(ahalberstadt) → review+
Assignee: nobody → james
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Sorry, should have marked this leaveopen since it isn't actually running anywhere yet.
Resolution: FIXED → ---
This was broken in a couple of different ways :( Attached a patch to fix things up a bit.
Attachment #8657218 - Flags: review?(jgriffin)
Attachment #8657218 - Flags: review?(jgriffin) → review+
Keywords: leave-open
So I tried enabling this on nightly/aurora and seem to have run into the builder limit:

  File "/home/jgraham/.mozilla/releng/repos/braindump/buildbot-related/", line 165, in <module>
  File "/home/jgraham/.mozilla/releng/repos/braindump/buildbot-related/", line 146, in main
    dump = dump_master(args.masters[0])
  File "/home/jgraham/.mozilla/releng/repos/braindump/buildbot-related/", line 111, in dump_master
    c = loadMaster(path)
  File "/home/jgraham/.mozilla/releng/repos/braindump/buildbot-related/", line 26, in loadMaster
    execfile(path, g, g)
  File "/home/jgraham/.mozilla/releng/repos/buildbot-configs/test-output/bm51-tests1-linux64/master.cfg", line 246, in <module>
    assert count <= builderLimit, "%s has %i builders; limit is %i" % (s, count, builderLimit)
AssertionError: tst-linux64-spot-2089 has 4104 builders; limit is 4084

Any ideas?
Flags: needinfo?(catlee)
I'm removing all the builders on gum in bug 1206269.  Once this is merged to production, you should have available builders to work with.
Flags: needinfo?(catlee)
Attached patch to enable tests on e10s-enabled trees. Obviously this depends on bug 1206016 to ensure that people can try their changes before landing to inbound.
Attachment #8664534 - Flags: review?(jlund)
Attached file List of builders added
Comment on attachment 8664534 [details] [diff] [review]
Enable wpt-e10s on default trees for nightly and aurora

Review of attachment 8664534 [details] [diff] [review]:

looks good to me :)
Attachment #8664534 - Flags: review?(jlund) → review+
Attached patch wpt-e10s.diffSplinter Review
Updated the patch to enable this to remove the windows builders on non-try due to capacity issues. Tested again with braindump.
Attachment #8664534 - Attachment is obsolete: true
Attachment #8671336 - Flags: review?(jlund)
Comment on attachment 8671336 [details] [diff] [review]

Review of attachment 8671336 [details] [diff] [review]:

Attachment #8671336 - Flags: review?(jlund) → review+
Bug 1197365 - Enable web-platform-tests-e10s tests on nightly and aurora branches, r=jlund
Now running on nightly + aurora branches, except on Windows where we are having a capacity crisis.
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.