Closed Bug 1413928 Opened 7 years ago Closed 6 years ago

[tryselect] Implement running tests by path for |mach try fuzzy|

Categories

(Testing :: General, enhancement)

Product:
Testing
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(7 files)

Bug 1413928 - [mozharness] Accept extra test harness args via environment variable
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1413928 - [tryselect] Add a new 'path' template
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1413928 - [tryselect] Create a new argument group for 'task' arguments
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1413928 - [tryselect] Change templates to return an entire context dict instead of a single value
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1413928 - [tryselect] Implement paths for |mach try fuzzy|
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1413928 - [ci] Refactor worker and platform out of python source test tasks
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1413928 - [tryselect] Add python unittest for templates
59 bytes, 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).
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.
Yeah, individual tests will work too.
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.
Depends on: 1414399
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|
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|
Depends on: 1412121
Bug 1412121 is on hold, so rather than blocking let's cherry-pick the changes we needed from there.
No longer depends on: 1412121
See Also: → 1400518
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.
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 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 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+
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 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 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 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 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 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 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 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.
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
Depends on: 1446884
Depends on: 1480142
Depends on: 1501359
No longer depends on: 1501359
Depends on: 1507633
Depends on: 1507634
Depends on: 1507635
No longer depends on: 1507633
Depends on: 1507767
Depends on: 1507779
Depends on: 1509045
Depends on: 1598776
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: