Closed Bug 1411660 Opened 2 years ago Closed 2 years ago

Add web-platform tests to test-verify in automation

Categories

(Testing :: General, enhancement)

enhancement
Not set

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: gbrown, Assigned: gbrown)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1405141 got things started by adding --verify support to the wpt harness. Now let's use it in automation. Most of this work is in mozharness.
I need to work out a couple of more issues before proceeding here, but most of that should be in mozharness -- the taskcluster stuff seems stable.

This adds a new task, "test-verify-wpt" with symbol "TVw" to desktop platforms. I'm adding this as tier 3 for now, until proven stable. This is similar to TV, but dedicated to web-platform tests. I cannot comfortably add web-platform support to the existing TV because wpt has its own mozharness script, rather than desktop_unittest.py.
Attachment #8925092 - Flags: review?(jmaher)
Comment on attachment 8925092 [details] [diff] [review]
task cluster config changes to add test-verify-wpt

Review of attachment 8925092 [details] [diff] [review]:
-----------------------------------------------------------------

this is fairly simple!
Attachment #8925092 - Flags: review?(jmaher) → review+
Apologies for all the delays here!

We need to finish off bug 1413729, but I'm happy with the patches that jgraham put together there. Together, this is looking good on try now, and doesn't break the existing test-verify, nor web-platform tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb7118af17fe65e32a2606cfbe323955bec7d209&filter-tier=1&filter-tier=2&filter-tier=3

Joel, looking for your review here since you've been following test verification from the start; James, you know the web-platform side.

In this patch, I'm adding web-platform test verification support to mozharness. The goal is to determine when a web-platform test is modified and then launch the test harness on each such test, with verification arguments added of course.
Attachment #8929286 - Flags: review?(jmaher)
Attachment #8929286 - Flags: review?(james)
Comment on attachment 8929286 [details] [diff] [review]
mozharness updates to extend test verification to web-platform tests

Review of attachment 8929286 [details] [diff] [review]:
-----------------------------------------------------------------

This took me longer than I thought to review- thanks for making this work in wpt!  From what I can tell all of the odd issues we have seen when it comes to test-verify will still be accounted for in wpt which is great news.
Attachment #8929286 - Flags: review?(jmaher) → review+
Comment on attachment 8929286 [details] [diff] [review]
mozharness updates to extend test verification to web-platform tests

Review of attachment 8929286 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this! I'm really excited to see it land. I think some followup work will be needed if we want to use this for stability checking test imports (which I would indeed like to do, since we end up with much of the same logic in the test import tool), but this is a great start.

::: testing/mozharness/mozharness/mozilla/testing/verify_tools.py
@@ +107,5 @@
> +        man = wptmanifest.manifest.load(tests_root, man_path)
> +
> +        repo_tests_path = os.path.join("testing", "web-platform", "tests")
> +        tests_path = os.path.join("tests", "web-platform", "tests")
> +        for (type, path, test) in man:

So, wpt also has a function to get a list of affected tests (and types) from a list of changed files, see [1]. We don't need to switch to that as a prerequisite to landing this, but it does try to do a little better than just checking which changed files are tests because it tries to figure out if the file is a support file that's used by some other test that will be run. Switching to that will make this work just like upstream in terms of the tests that are run, which is what we ideally want.

[1] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wpt/testfiles.py#261-286

@@ +189,5 @@
>              files = self.verify_suites.get(suite)
>              references = re.compile(r"(-ref|-noref|-noref.)\.")
>              for file in files:
> +                if self.config.get('verify_category') == "web-platform":
> +                    args.append(['--verify-log-full', '--verify', file])

How does this work? Do we do a separate run per test? That seems like it isn't going to scale well; from experience with wpt imports we can sometimes have hundreds or thousands of test changes in a single push. It might be nice to (at least optionally) run all the tests in the same harness invocation to account for cases like that (in the future I would also like to be able to extend the maximum time using e.g. a try argument so that we can use this for importing wpt PRs.
Attachment #8929286 - Flags: review?(james) → review+
(In reply to James Graham [:jgraham] from comment #7)
> So, wpt also has a function to get a list of affected tests (and types) from
> a list of changed files, see [1].

Thanks for that - I wasn't aware of it. I'm resisting the urge to switch to that immediately: filed bug 1418363.

> Do we do a separate run per test? That seems like it
> isn't going to scale well

Yes, it is a separate test harness run per test. I've filed bug 1418375 for follow-up on this and other scaling/import concerns.
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f01cc9399e6
Schedule test-verify-wpt on all desktop platforms, tier 3; r=jmaher
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b960167158f
mozharness changes to support test-verify-wpt; r=jmaher,jgraham
https://hg.mozilla.org/mozilla-central/rev/5f01cc9399e6
https://hg.mozilla.org/mozilla-central/rev/5b960167158f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.