Closed Bug 1316877 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla53

People

(Reporter: dustin, Assigned: dustin)

Details

Attachments

(1 file)

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.
Attachment #8816847 - Flags: review?(jseward) → review?(jmaher)
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'?
Attachment #8816847 - Flags: review?(jmaher) → review-
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 :)
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.
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 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:)
Attachment #8816847 - Flags: review?(jmaher) → review+
We're changing test sets rapidly!  Mozreview keeps failing to land it due to conflicts..
https://hg.mozilla.org/mozilla-central/rev/f427b837088a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.