Closed
Bug 1413928
Opened 7 years ago
Closed 7 years ago
[tryselect] Implement running tests by path for |mach try fuzzy|
Categories
(Testing :: General, enhancement)
Testing
General
Tracking
(firefox59 fixed)
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: ahal, Assigned: ahal)
References
(Depends on 2 open bugs, Blocks 1 open bug)
Details
Attachments
(7 files)
(deleted),
text/x-review-board-request
|
impossibus
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
impossibus
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
impossibus
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
impossibus
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
impossibus
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jmaher
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
davehunt
:
review+
|
Details |
We already have the ability to pass in paths via |mach try syntax|, but the implementation is fairly hacky, it's missing many suites/platforms and there are several reports of it being broken for various configurations.
I'd like to re-imagine this feature as a new |mach try| subcommand, e.g:
./mach try path dom/indexedDB dom/cache
Instead of relying on mozharness to do the suite/test resolving, we would perform this step locally and make use of the new try_task_config.json + json-e templates to modify the tasks so only tests under the specified directories get run.
The test resolving step could be identical to what |mach test| does (we could factor this code out to mozbase or somewhere). This way running |mach test <path>| and |mach try <path>| would run the same set of tests (the former locally, and the latter in CI).
Comment 1•7 years ago
|
||
That sounds very useful.
I would like to see individual test files allowed as <path> possibilities: |mach try path dom/indexedDB/test/test_count.html|. That wouldn't be the main/normal use case, but would be useful for debug-on-try scenarios where there is suspicion that another test in the directory is interfering with browser state, etc.; or, perhaps when the test of interest is buried in a big directory of tests and snappy turn-around is desired.
Assignee | ||
Comment 2•7 years ago
|
||
Yeah, individual tests will work too.
Assignee | ||
Comment 3•7 years ago
|
||
I have a working prototype:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd04bd869a181c2ed5f81dc9252288ac720a5b0b
Still a lot of edge cases and the patch series it depends on is pretty huge. I'll file some dependent bugs to get a bunch of the legwork done ahead of time.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Here's the result of |mach try path caps|:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d3abf466a851fbfe0af18a2bb210edcc80f314f
Much of the prerequisite build/test resolving work landed in bug 1414399. But there's still a fair bit left to do here:
* Support MOZHARNESS_TEST_PATHS in other mozharness scripts (marionette, wpt, android, etc)
* Fill out the rest of the 'task_regexes' (and decide if regexes are even the best approach here)
* Also use templates to change the treeherder group/symbol to something more helpful
* Better default handling (so |mach try <path>| dispatches to |mach try path <path>|)
* Create a test for |mach try path|
Assignee | ||
Comment 10•7 years ago
|
||
Thinking about this a bit more, it makes more sense to just have this be an option for the other subcommands. That way we won't need to reinvent the wheel for filtering down platforms, etc. Since this feature technically exists for 'syntax' already, I'll focus on 'fuzzy' to start. The 'syntax' implementation is kind of broken, so we can switch to this new implementation in a follow-up bug.
As far as the workflow for |mach try fuzzy|, I think if a path is specified we filter down the task list using the regexes and only display those in fzf. So, e.g to run all tests under dom/indexedDB on linux only:
./mach try fuzzy dom/indexedDB -q linux
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Summary: [tryselect] Implement |mach try path| → [tryselect] Implement running tests by path for |mach try fuzzy|
Assignee | ||
Comment 11•7 years ago
|
||
Bug 1412121 is on hold, so rather than blocking let's cherry-pick the changes we needed from there.
No longer depends on: 1412121
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
I've been chipping away at this slowly and am getting close. Should have it ready for review sometime this week, just need to do a bit more testing and cleanup.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•7 years ago
|
||
Maja: Sorry, I know you aren't familiar with this stuff but Armen's still on vacation (and he's not very familiar with this either). So you "win" the review lottery because you expressed interest in seeing this finished awhile back :). But I don't mind finding someone else if you'd prefer.
Dave: I'm flagging you on this commit because I'm using some weird pytest features I found on the internet.
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8942759 [details]
Bug 1413928 - [ci] Refactor worker and platform out of python source test tasks
https://reviewboard.mozilla.org/r/213012/#review218924
::: taskcluster/ci/source-test/python.yml:11
(Diff revision 2)
> linux64.*: aws-provisioner-v1/gecko-t-linux-xlarge
> + worker:
> + by-platform:
> + linux64.*:
> + docker-image: {in-tree: "lint"}
> + max-run-time: 3600
many of the line removals have a |platform:| section and that had linux64, now this is not here- how can we assure that all of these tests are linux64 only?
Attachment #8942759 -
Flags: review?(jmaher) → review-
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8942760 [details]
Bug 1413928 - [tryselect] Add python unittest for templates
https://reviewboard.mozilla.org/r/213014/#review218934
This looks okay, though I wonder if you could use @pytest.mark.parametrize instead of pytest_generate_tests? See https://docs.pytest.org/en/latest/parametrize.html for more info.
Attachment #8942760 -
Flags: review?(dave.hunt) → review+
Assignee | ||
Comment 31•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8942759 [details]
Bug 1413928 - [ci] Refactor worker and platform out of python source test tasks
https://reviewboard.mozilla.org/r/213012/#review218924
> many of the line removals have a |platform:| section and that had linux64, now this is not here- how can we assure that all of these tests are linux64 only?
``platform: linux64/opt`` is in the job-defaults section at the top of the file. This means it'll be used by default if not explicitly specified in the task config. But it might be better to just be explicit with the platform, if you like I can undo those changes just for readability. I don't have a strong preference either way.
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8942759 [details]
Bug 1413928 - [ci] Refactor worker and platform out of python source test tasks
https://reviewboard.mozilla.org/r/213012/#review218962
after discussing on irc and looking at the file, we already had platform:linux64.
Attachment #8942759 -
Flags: review- → review+
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8928705 [details]
Bug 1413928 - [mozharness] Accept extra test harness args via environment variable
https://reviewboard.mozilla.org/r/199954/#review219004
Attachment #8928705 -
Flags: review?(mjzffr) → review+
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8928708 [details]
Bug 1413928 - [tryselect] Change templates to return an entire context dict instead of a single value
https://reviewboard.mozilla.org/r/199960/#review219006
Attachment #8928708 -
Flags: review?(mjzffr) → review+
Comment 35•7 years ago
|
||
mozreview-review |
Comment on attachment 8928706 [details]
Bug 1413928 - [tryselect] Add a new 'path' template
https://reviewboard.mozilla.org/r/199956/#review219008
Attachment #8928706 -
Flags: review?(mjzffr) → review+
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8928707 [details]
Bug 1413928 - [tryselect] Create a new argument group for 'task' arguments
https://reviewboard.mozilla.org/r/199958/#review219014
Attachment #8928707 -
Flags: review?(mjzffr) → review+
Comment 37•7 years ago
|
||
mozreview-review |
Comment on attachment 8928709 [details]
Bug 1413928 - [tryselect] Implement paths for |mach try fuzzy|
https://reviewboard.mozilla.org/r/199962/#review219018
::: testing/mozbase/moztest/moztest/resolve.py:153
(Diff revision 3)
> },
> 'steeplechase': {},
> 'web-platform-tests': {
> 'mach_command': 'web-platform-tests',
> 'kwargs': {'include': []},
> + 'task_regex': 'web-platform-tests(?:-e10s)?(?:-1)?$',
It would probably make sense to also include web-platform-tests-reftests and web-platform-tests-wdspec in the task regex for web-platform-tests. (These tasks also apply to tests under testing/web-platform/tests and can be run with ./mach wpt)
::: tools/tryselect/docs/selectors/fuzzy.rst:100
(Diff revision 3)
> + {
> + "templates":{
> + "env":{
> + "MOZHARNESS_TEST_PATHS":"layout/reftests/reftest-sanity"
> + }
> + },
> + "tasks":[
> + "test-linux64-qr/debug-reftest-e10s-1",
> + "test-linux64-qr/opt-reftest-e10s-1",
> + "test-linux64-stylo-disabled/debug-reftest-e10s-1",
> + "test-linux64-stylo-disabled/opt-reftest-e10s-1",
> + "test-linux64/debug-reftest-e10s-1",
> + "test-linux64/debug-reftest-no-accel-e10s-1",
> + "test-linux64/debug-reftest-stylo-e10s-1",
> + "test-linux64/opt-reftest-e10s-1",
> + "test-linux64/opt-reftest-no-accel-e10s-1",
> + "test-linux64/opt-reftest-stylo-e10s-1"
> + ]
> + }
Out of curiousity, is it valid to send a try_task_config.json that pairs different envs with different tasks to express something like "run these tasks with this path and these other tasks with this different path" on one try push?
Attachment #8928709 -
Flags: review?(mjzffr) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 45•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8928709 [details]
Bug 1413928 - [tryselect] Implement paths for |mach try fuzzy|
https://reviewboard.mozilla.org/r/199962/#review219018
> It would probably make sense to also include web-platform-tests-reftests and web-platform-tests-wdspec in the task regex for web-platform-tests. (These tasks also apply to tests under testing/web-platform/tests and can be run with ./mach wpt)
Makes sense, thanks!
> Out of curiousity, is it valid to send a try_task_config.json that pairs different envs with different tasks to express something like "run these tasks with this path and these other tasks with this different path" on one try push?
That's definitely a valid use case, though not possible with this patch. I don't see why we couldn't get something like that working in the future though. The hard part is probably, how do you provide a sane UI that allows users to specify that? The current fzf interface would fall pretty flat.
Though I suspect you'd be more interested in creating the ``try_task_config.json`` file manually anyway.
Comment 46•7 years ago
|
||
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7056a096843a
[mozharness] Accept extra test harness args via environment variable r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/7ee4038b4628
[ci] Refactor worker and platform out of python source test tasks r=jmaher
https://hg.mozilla.org/integration/autoland/rev/7e910ce230d0
[tryselect] Add python unittest for templates r=davehunt
https://hg.mozilla.org/integration/autoland/rev/0c677fb018be
[tryselect] Change templates to return an entire context dict instead of a single value r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/f1a0ec3a9177
[tryselect] Add a new 'path' template r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/959c0a8a3459
[tryselect] Create a new argument group for 'task' arguments r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/4d4c68ba3b03
[tryselect] Implement paths for |mach try fuzzy| r=maja_zf
Comment 47•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7056a096843a
https://hg.mozilla.org/mozilla-central/rev/7ee4038b4628
https://hg.mozilla.org/mozilla-central/rev/7e910ce230d0
https://hg.mozilla.org/mozilla-central/rev/0c677fb018be
https://hg.mozilla.org/mozilla-central/rev/f1a0ec3a9177
https://hg.mozilla.org/mozilla-central/rev/959c0a8a3459
https://hg.mozilla.org/mozilla-central/rev/4d4c68ba3b03
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•