1316877 - Allow `test-sets` in `test-platforms.yml`

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

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

In bug 1254964 and in a few other cases, we've seen the need to have some platforms run the "normal" tests *plus* a few others.  The current solution is to make two test sets and then keep the tests in those lists in sync.  That kinda stinks.  Let's allow multiple test sets to be specified, then take the union of the tests in those sets.

Comment 1

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

Attachment: Bug 1316877: allow 'test-sets' in test-platforms.yml;

This allows either `test-set` or `test-sets`, adding a simple layer of
indirection to avoid needless repetition of long lists of tests.

Review commit: https://reviewboard.mozilla.org/r/97372/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97372/

Comment 2

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

Comment on attachment 8816847 [details]
Bug 1316877: allow 'test-sets' in test-platforms.yml;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97372/diff/1-2/

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8816847 [details]
Bug 1316877: allow 'test-sets' in test-platforms.yml;

https://reviewboard.mozilla.org/r/97372/#review101012

::: taskcluster/ci/desktop-test/test-platforms.yml:38
(Diff revision 2)
> +        - external-media-tests
> +        - web-platform-tests
> +
>  linux64-asan/opt:
>      build-platform: linux64-asan/opt
> -    test-set: asan-tests
> +    test-set: common-tests

I would prefer we make this test-sets even if it is singular- do explain if you have other reasons for supporting singular and plural.

::: taskcluster/taskgraph/task/test.py:104
(Diff revision 2)
>          Returns a dictionary like `get_test_platforms`, with an additional
>          `test-names` key for each test platform, containing a set of test
>          names."""
>          rv = {}
>          for test_platform, cfg in test_platforms.iteritems():
> -            test_set = cfg['test-set']
> +            if 'test-set' not in cfg and 'test-sets' not in cfg:

why do you need to support 'test-set' in addition to 'test-sets'?

Comment 4

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

Comment on attachment 8816847 [details]
Bug 1316877: allow 'test-sets' in test-platforms.yml;

https://reviewboard.mozilla.org/r/97372/#review101012

> I would prefer we make this test-sets even if it is singular- do explain if you have other reasons for supporting singular and plural.

Just because it reads better, really.  Why do you prefer a plural label for a single value?  I don't mind changing it if you feel strongly :)

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

while it is a single value now, it could be a plural value in the future, I can see confusion in editing that.  also we now have to support two sets of tags, it gets confusing as to which tag to use.  So it is ok to have both, I would just prefer a single tag that supports everything.

Comment 6

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

It's not always a singular value.  Some are singular and some are plural:

    test-set: common-tests

or

    test-sets:
        - common-tests
        - external-media-tests
        - web-platform-tests

It doesn't seem any more confusing than writing a well-formed English sentence :)

If we switch to only allowing `test-sets`, then we need to look at the type of its value (list or string) so it doesn't really save any complexity in the implementation.  That said, it's a silly thing to continue to discuss, so I'll just do that.

Comment 7

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

Comment on attachment 8816847 [details]
Bug 1316877: allow 'test-sets' in test-platforms.yml;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97372/diff/2-3/

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8816847 [details]
Bug 1316877: allow 'test-sets' in test-platforms.yml;

https://reviewboard.mozilla.org/r/97372/#review101066

thanks Dustin- this sets us up well for the future:)

Comment 9

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

Comment on attachment 8816847 [details]
Bug 1316877: allow 'test-sets' in test-platforms.yml;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97372/diff/3-4/

Comment 10

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

We're changing test sets rapidly!  Mozreview keeps failing to land it due to conflicts..

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f427b837088a13502f60a2c77faf76387b0b03d0
Bug 1316877: allow 'test-sets' in test-platforms.yml; r=jmaher

Comment 12

[Phil Ringnalda (:philor)]

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