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

RESOLVED FIXED in mozilla53

Status

RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: dustin, Assigned: dustin)

Tracking

unspecified
mozilla53

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8816847 - Flags: review?(jseward) → review?(jmaher)

Comment 3

2 years ago
mozreview-review
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-
(Assignee)

Comment 4

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
We're changing test sets rapidly!  Mozreview keeps failing to land it due to conflicts..

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f427b837088a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

8 months ago
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.