1213314 - Taskcluster try syntax (near) parity with buildbot

Status: RESOLVED FIXED

(Taskcluster :: General, defect)

Description

[Andrew Halberstadt [:ahal]]

(Not sure what component this belongs in).

Taskcluster's try parser [1] is distinct from buildbot's [2]. If we're going to enabled taskcluster tests as tier-1 jobs, people will expect the try syntax to work similarly.

Here's an example I ran into while working on bug 1171033:

With buildbot, 'try: -b o -p linux64 -u mochitest' will run all variants (bc, dt, gl etc). On taskcluster, this will only run mochitest plain. To get the same thing you'd need to use 'try: -b o -p linux64 -u mochitest,mochitest-bc,mochitest-dt,mochitest-gl'.

Currently the taskcluster try_parser will use the job keys in the branch configuration to match against the try string. This is inflexible and doesn't lend itself well to aliases. For example, if you want 'mochitest-bc' to work, you can't call the job 'mochitest-browser-chrome' in the branch configs.

It would be super awesome if try aliases could be defined independently from the root key, as a list of rules with the ability to use operators like '==', 'in' or 'startswith'.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/taskcluster_graph/try_test_parser.py
[2] http://hg.mozilla.org/build/buildbotcustom/file/tip/try_parser.py

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

Yikes.  try_parser.py is scary, and seems to be deeply bound up in the naming of builders.  NOT MY CIRCUS, NOT MY MONKEYS.

I had hoped to solve this with a TDD approach, throwing a bunch of sample try syntax at both the old and new parsers, and making sure that they produce the same output.  But the output for one would be builder names, and for the other YAML files and a few parameters like chunk number.

So I'm going to scope this down to just fixing the test-name expansion Andrew mentioned.  This is implemented at http://hg.mozilla.org/build/buildbotcustom/file/tip/try_parser.py#l14.  There's also the issue of filtering test platforms (e.g., -u mochitest[10.6]), but since we don't have any test platforms to filter yet, I'm not going to worry about that right now.

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

Oh hey, the filtering is already implemented!

Comment 3

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: MozReview Request: Bug 1213314: expand try alias support and implement aliases; r?ahal

Bug 1213314: expand try alias support and implement aliases; r?ahal

This adds a lot of functionality to the `flags.aliases` field in the try
comment parser, and implements all of the aliases currently supported by
Buildbot's try_parser.py.

The situation changes slightly because of the way chunks are handled; it's no
longer possible to specify chunks using an alias with no leading `-`.  This
change should not cause undue hardship.

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8677710 [details]
MozReview Request: Bug 1213314: expand try alias support and implement aliases; r?ahal

Bug 1213314: expand try alias support and implement aliases; r?ahal

This adds a lot of functionality to the `flags.aliases` field in the try
comment parser, and implements all of the aliases currently supported by
Buildbot's try_parser.py.

The situation changes slightly because of the way chunks are handled; it's no
longer possible to specify chunks using an alias with no leading `-`.  This
change should not cause undue hardship.

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8677710 [details]
MozReview Request: Bug 1213314: expand try alias support and implement aliases; r?ahal

https://reviewboard.mozilla.org/r/23007/#review20581

Thanks! I agree this is most likely good enough. I don't think we should get too caught up trying to replicate the old try parser, as the old try parser is pretty terrible. My hope is that try syntax just dies completely some day anyway (replaced by tools calling buildapi).

::: testing/taskcluster/taskcluster_graph/commit_parser.py:104
(Diff revision 2)
> +def handle_alias(test, test_name, aliases, all_tests):

nit: no need to have 'test_name' as a separate argument. Can do test_name = test['test'] instead.

::: testing/taskcluster/taskcluster_graph/commit_parser.py:129
(Diff revision 2)
> +def parse_test_chunks(aliases, all_tests, tests):
>      '''
>      Test flags may include parameters to narrow down the number of chunks in a
>      given push. We don't model 1 chunk = 1 job in taskcluster so we must check
>      each test flag to see if it is actually specifying a chunk.
>  
>      :param dict aliases: Dict of alias name -> real name.
>      :param list tests: Result from normalize_test_list
>      :returns: List of jobs
>      '''

nit: Please update the docstring so the difference between 'all_tests' and 'tests' is clear.

::: testing/taskcluster/tasks/branches/base_job_flags.yml:34
(Diff revision 2)
> +    mochitest-o: /.*mochitest-o.*/
> +    mochitest-a11y: /.*mochitest-o.*/

When bug 1211889 is live (which should be very soon), this will no longer be the case. I don't know what the new try syntax will be though.

I'd be fine punting on this if you prefer.

::: testing/taskcluster/tests/test_commit_parser.py:52
(Diff revision 2)
> +    def test_normalize_test_list_with_alias_pattern(self):

Thanks for the tests!

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

https://reviewboard.mozilla.org/r/23007/#review20597

nits nitted

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e7d5986ace

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d8e7d5986ace

Comment 9

[Andrew Halberstadt [:ahal]]

Somehow this change causes mochitest-browser-chrome, mochitest-devtools-chrome and mochitest-webgl to get scheduled twice on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b84d34d7166a&filter-searchStr=mochitest

You can test it out locally by running:
> ./mach taskcluster-graph --project=try --message='try: -b o -p linux64 -u all -t none' --owner [e-mail] --extend-graph --print-names-only
both with and without this change.

Comment 10

[Andrew Halberstadt [:ahal]]

Apologies, the duplicate scheduling was actually not regressed by this change, it existed before. This change just makes it easier to specify a try syntax that triggers the bug.

For example, even without this patch if you run a try push with the syntax:
try: -b o -p linux64-mulet -u mochitest,mochitest -t none

All the mulet mochitests will get scheduled twice. The fix probably involves using a set instead of a list. To keep things a little more conceptually maintainable, I'll file a new bug.

Comment 11

[Armen [:armenzg]]

This is not working for me at the moment.

See this for example:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0510aa4c5095

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

can you elaborate?

Comment 13

[Armen [:armenzg]]

I used try: -b o -p linux64 -u jittests,xpcshell -t none, however, only the build got scheduled.
https://tools.taskcluster.net/task-graph-inspector/#UE-hg-TiQZil1tL4U8qcdA/

If I use -u all, it will schedule the jittests and the xpcshell tests.

Comment 14

[Dustin J. Mitchell [:dustin] (he/him)]

We don't currently support tests run against opt builds:

dustin@euclid ~/code/moz/t/m-c $ hg up -r 0510aa4c509556aad819c98383091cafa23e1981
158 files updated, 0 files merged, 18 files removed, 0 files unresolved
(leaving bookmark bug1221661)

dustin@euclid ~/code/moz/t/m-c $ ./mach taskcluster-graph --pushlog-id=101192 --project=try --message='try: -b o -p linux64 -u jittests,xpcshell -t none' --owner='[e-mail]' --revision-hash='1716b4a233d47d78ab6f2ea52d5ee3d99037504d' --print-names-only
Querying URL for pushdate: None/json-pushes?changeset=None
Error querying pushinfo for repository 'None' revision 'None'
- [TC] Linux64 Opt

dustin@euclid ~/code/moz/t/m-c $ ./mach taskcluster-graph --pushlog-id=101192 --project=try --message='try: -b d -p linux64 -u jittests,xpcshell -t none' --owner='[e-mail]' --revision-hash='1716b4a233d47d78ab6f2ea52d5ee3d99037504d' --print-names-only
Querying URL for pushdate: None/json-pushes?changeset=None
Error querying pushinfo for repository 'None' revision 'None'
- [TC] Linux64 Dbg
  - [TC] Linux64 jittests 1
  - [TC] Linux64 jittests 2
  - [TC] Linux64 xpcshell 1
  - [TC] Linux64 xpcshell 2
  - [TC] Linux64 xpcshell 3
  - [TC] Linux64 xpcshell 4

Comment 15

[Armen [:armenzg]]

My apologies about the confusion :/