987360 - Ability to tag tests and run all tests having a tag

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Gregory Szorc [:gps]]

There are a number of features that have tests scattered across multiple directory trees. It would be rad if I could run all tests for a given feature without having to enumerate all the directories/test files related to that feature on the command line.

e.g. |mach test --feature sync| would run all Sync-related tests.

I think the way to facilitate this is to introduce a "tagging" mechanism into test manifests. We could add a "tag" field to manifests (either globally in the [DEFAULT] section or part of an individual test file. This "tag" field would be a set of space or comma delimited words denoting tags.

Once we have tag metadata associated with tests, we can teach the mach testing commands to filter tests by tags.

If "tags" is too generic, I reckon we could call this "feature".

Is this a good idea?

Comment 1

[(not currently active) Ted Mielczarek]

Funny, we talked about something very similar to this in bug 984930.

Comment 2

[Richard Newman [:rnewman]]

This would have caught several bugs for me over the past few years. +1.

Comment 3

[Gregory Szorc [:gps]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> Funny, we talked about something very similar to this in bug 984930.

Funny, I was just about to comment that if you implement a query language for selecting tests, an automation job could effectively be reduced down to a query that |mach test| knows how to evaluate :)

Comment 4

[Gregory Szorc [:gps]]

Attachment: Test tagging

I submitted a proof-of-concept for people to comment on. It isn't perfect, but it works!

Comment 5

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

I am unable to login to reviewboard (error 500), please upload the patch to bugzilla so I can make comments on it inline.

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 8396906 [details]
Test tagging

I'm not really sure what the proper RB+BZ workflow is, but commented on RB.

Comment 7

[Andrew Halberstadt [:ahal]]

(In reply to Joel Maher (:jmaher) from comment #5)
> I am unable to login to reviewboard (error 500), please upload the patch to
> bugzilla so I can make comments on it inline.

I got this when trying to log in with my LDAP (e.g persona) login. It worked when I used my actual bugzilla username password, which is separate from the persona login.

Comment 8

[Andrew Halberstadt [:ahal]]

I can't log into reviewboard.allizom.org anymore, but the proposed patch is likely outdated as I've recently re-vamped manifestparser a lot. The new way to implement a 'tag' field would be to add a new built-in filter to manifestparser:
https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestparser/manifestparser/filters.py

Comment 9

[Gregory Szorc [:gps]]

Attachment: MozReview Request: bz://987360/gps

/r/5465 - Bug 987360 - Ability to tag tests and to run tagged tests

Pull down this commit:

hg pull review -r 9a31d373d7149b46344368bea14318756bd432cd

Comment 10

[Gregory Szorc [:gps]]

I just pushed the old, and likely bitrotted patch.

I don't have time to work on this now. Someone, please, take over this work!

Comment 11

[Andrew Halberstadt [:ahal]]

I'd like to see that logic going into manifestparser/test harnesses rather than (or in addition to) mozbuild. I'll likely pick this up in the near(ish) future if no one else does.

Comment 12

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: bz://987360/ahal

/r/5895 - Bug 987360 - Add ability to tag tests with arbitrary strings and run them, r=chmanchester

Pull down this commit:

hg pull review -r 2a5f8af1aa5f8c8860fe1b140642fafc23ea121f

Comment 13

[Andrew Halberstadt [:ahal]]

This adds a --tag parameter to mochitest, xpcshell and marionette (cppunittests is missing because it doesn't use manifest.ini with mach, which is non-trivial to fix). This patch aligns nicely with the recent test chunking I've been doing to the test harnesses.

Gps, this doesn't touch the build system at all, but feel free to give feedback if you think this is the wrong approach.

Comment 14

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/5893/#review4891

I think this approach looks fine!

Comment 15

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8581922 [details]
MozReview Request: bz://987360/ahal

https://reviewboard.mozilla.org/r/5893/#review5075

::: testing/marionette/client/marionette/runner/base.py
(Diff revision 1)
> +                        help="filter out tests that don't have the given tag. Can be "

nit: s/filter/Filter/

::: testing/mozbase/manifestparser/manifestparser/filters.py
(Diff revision 1)
>      def __init__(self, this_chunk, total_chunks, disabled=False):
>          assert 1 <= this_chunk <= total_chunks
> +        InstanceFilter.__init__(self, this_chunk, total_chunks,
> +                                disabled=disabled)

Looks like we're passing arguments to the superclass it doesn't really care about except so it can print them out. For the case of non-kwargs aren't the messages going to be unhelpful anyways? I can't think of a better way off the top of my head, but seems a little wonky.

::: testing/xpcshell/runxpcshelltests.py
(Diff revision 1)
> +from manifestparser.filters import tags as filter_tags

How about making this consistent? I'd be for naming the filter itself or the command line "test_tags" or "run_tags" or something so we don't have this difference between harnesses and statements like 

     if self.tags:
         filters.append(tags(self.tags))
         
from the marionette case seem slightly less magical at first reading.

::: testing/xpcshell/runxpcshelltests.py
(Diff revision 1)
> +        if len(self.alltests) == 0:
> +            self.log.error("no tests to run using specified "
> +                           "combination of filters: {}".format(
> +                                mp.fmt_filters()))
> +            sys.exit(1)

This is a new behavior for our harnesses. If we were paranoid about crazy workflows that depend on the ability to run no tests we could just log the error and carry on.

Comment 16

[Andrew Halberstadt [:ahal]]

Comment on attachment 8581922 [details]
MozReview Request: bz://987360/ahal

/r/5895 - Bug 987360 - Add ability to tag tests with arbitrary strings and run them, r=chmanchester

Pull down this commit:

hg pull review -r 7e8424aa3efcc16c2d7376377ee1f55407157d13

Comment 17

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/5893/#review5117

> Looks like we're passing arguments to the superclass it doesn't really care about except so it can print them out. For the case of non-kwargs aren't the messages going to be unhelpful anyways? I can't think of a better way off the top of my head, but seems a little wonky.

Oh, there was a change missing here! I agree it is a bit wonky, but also not sure of any better way.

Comment 18

[Andrew Halberstadt [:ahal]]

Not sure why mozreview is showing those 'with marionette' changes in base.py, they aren't in my local commit.

Comment 19

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8581922 [details]
MozReview Request: bz://987360/ahal

https://reviewboard.mozilla.org/r/5893/#review5149

Excellent!

Comment 20

[Andrew Halberstadt [:ahal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c34fd480729

I'll do a dev.platform post on this Monday once/if it gets merged to central.

Comment 21

[Andrew Halberstadt [:ahal]]

I accidentally broke xpcshell on b2g/android. Here's a bustage fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acbab9e22691

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Still broken. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1b2cb81a3f3

https://treeherder.mozilla.org/logviewer.html#?job_id=8147751&repo=mozilla-inbound

Comment 23

[Andrew Halberstadt [:ahal]]

Comment on attachment 8581922 [details]
MozReview Request: bz://987360/ahal

/r/5895 - Bug 987360 - Add ability to tag tests with arbitrary strings and run them, r=chmanchester

Pull down this commit:

hg pull review -r d75905da1ca021a15a9538117d3866425f73d962

Comment 24

[Andrew Halberstadt [:ahal]]

Comment on attachment 8581922 [details]
MozReview Request: bz://987360/ahal

Sorry about that, it looked like a trivial fix at first glance but there was some inheritance at play. The latest mozreview push has the proper fix. And here's a try run that includes android/b2g this time:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9c6648f48f4

Carrying r+ forward.

Comment 25

[Andrew Halberstadt [:ahal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb698f70e79

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7fb698f70e79

Comment 27

[Andrew Halberstadt [:ahal]]

Attachment: Enable chunk-by-runtime on try

Here's a patch to get this running on try. So far only mochitest browser-chrome and devtools data is checked into the tree, so only those suites will work.

Comment 28

[Andrew Halberstadt [:ahal]]

Comment on attachment 8586073 [details] [diff] [review]
Enable chunk-by-runtime on try

Sorry, wrong bug..

Comment 29

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

It seems multiple tags can be added comma separarated. It seems setting multiple "head" files works space separated. Any reason this needs to be different?

Comment 30

[Andrew Halberstadt [:ahal]]

No, good point. I think support-files is also space delimited. I'll patch it up before it gets heavy use.

Comment 32

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Bug 987360 - Add ability to tag tests with arbitrary strings and run them, r=chmanchester

Comment 33

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 987360 - Ability to tag tests and to run tagged tests