Closed
Bug 1169410
Opened 9 years ago
Closed 9 years ago
Add wpt support to |mach test|
Categories
(Testing :: web-platform-tests, defect)
Testing
web-platform-tests
Tracking
(firefox41 fixed)
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: jgraham, Assigned: jgraham)
Details
Attachments
(1 file)
Should allow mach test web-platform-tests mach test wpt mach test testing/web-platform/tests/dom/historical.html mach test /dom/historical.html
Assignee | ||
Comment 1•9 years ago
|
||
Bug 1169410 Add support for web-platform-tests to |mach test|
Attachment #8612509 -
Flags: review?(gps)
Updated•9 years ago
|
Assignee: nobody → james
Comment 2•9 years ago
|
||
Comment on attachment 8612509 [details] MozReview Request: Bug 1169410 Add support for web-platform-tests to |mach test| https://reviewboard.mozilla.org/r/9589/#review8559 This looks mostly good. Just fix up the import code and this should be an easy r+ on the follow-up. ::: python/mozbuild/mozbuild/frontend/context.py:1303 (Diff revision 1) > + """List of [manifest_path, test_path] defining web-platform-tests. Nit: Should use () not [] to denote tuples in docs. ::: python/mozbuild/mozbuild/frontend/emitter.py:1189 (Diff revision 1) > + old_path = sys.path[:] > + paths = [] > + paths_file = os.path.join(context.config.topsrcdir, "testing", > + "web-platform", "tests", "tools", "localpaths.py") > + _globals = {"__file__": paths_file} > + execfile(paths_file, _globals) > + import manifest as wptmanifest > + sys.path = old_path This definitely requires some comments. ::: python/mozbuild/mozbuild/frontend/emitter.py:1196 (Diff revision 1) > + sys.path = old_path This should be a try..finally. ::: python/mozbuild/mozbuild/frontend/emitter.py:1198 (Diff revision 1) > + manifest = wptmanifest.manifest.load(tests_root, manifest_full_path) How about extracting this logic for obtaining the wptmanifest module into a standalone function. While you are there, add some caching so we don't do all this work for every wpt manifest. ::: python/mozbuild/mozbuild/frontend/emitter.py:1219 (Diff revision 1) > + 'head': '', > + 'tail': '', > + 'support-files': '', Using empty strings seems weird. Are you sure we do that instead of using [] or None? ::: testing/web-platform/moz.build:7 (Diff revision 1) > +WEB_PLATFORM_TESTS_MANIFESTS += [('meta/MANIFEST.json', 'tests/'), > + ('mozilla/meta/MANIFEST.json', 'mozilla/tests/')] Nit: newline after the [. ::: testing/web-platform/mach_commands.py:131 (Diff revision 1) > + del params["test_objects"] I'm not a huge fan of mutating input like this. But it is probably acceptable.
Attachment #8612509 -
Flags: review?(gps)
Assignee | ||
Comment 3•9 years ago
|
||
https://reviewboard.mozilla.org/r/9589/#review8561 > How about extracting this logic for obtaining the wptmanifest module into a standalone function. While you are there, add some caching so we don't do all this work for every wpt manifest. In practice this function is only called once, so I don't think that adding caching is a win. > Using empty strings seems weird. Are you sure we do that instead of using [] or None? I literally just copied the reftests code above.
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8612509 [details] MozReview Request: Bug 1169410 Add support for web-platform-tests to |mach test| Bug 1169410 Add support for web-platform-tests to |mach test|
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8612509 [details] MozReview Request: Bug 1169410 Add support for web-platform-tests to |mach test| Bug 1169410 Add support for web-platform-tests to |mach test|
Attachment #8612509 -
Flags: review?(gps)
Updated•9 years ago
|
Attachment #8612509 -
Flags: review?(gps) → review+
Comment 6•9 years ago
|
||
Comment on attachment 8612509 [details] MozReview Request: Bug 1169410 Add support for web-platform-tests to |mach test| https://reviewboard.mozilla.org/r/9589/#review8765 Ship It!
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ad8d961bafef
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•