Closed Bug 987360 Opened 6 years ago Closed 5 years ago

Ability to tag tests and run all tests having a tag

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: gps, Assigned: ahal)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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?
Funny, we talked about something very similar to this in bug 984930.
This would have caught several bugs for me over the past few years. +1.
(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 :)
Attached file Test tagging (obsolete) —
I submitted a proof-of-concept for people to comment on. It isn't perfect, but it works!
Attachment #8396906 - Flags: feedback?(ahalberstadt)
I am unable to login to reviewboard (error 500), please upload the patch to bugzilla so I can make comments on it inline.
Comment on attachment 8396906 [details]
Test tagging

I'm not really sure what the proper RB+BZ workflow is, but commented on RB.
Attachment #8396906 - Flags: feedback?(ahalberstadt) → feedback+
(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.
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
Blocks: chunking
Attached file MozReview Request: bz://987360/gps (obsolete) —
/r/5465 - Bug 987360 - Ability to tag tests and to run tagged tests

Pull down this commit:

hg pull review -r 9a31d373d7149b46344368bea14318756bd432cd
Attachment #8396906 - Attachment is obsolete: true
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!
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.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Attached file MozReview Request: bz://987360/ahal (obsolete) —
/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
Attachment #8581922 - Flags: review?(cmanchester)
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.
https://reviewboard.mozilla.org/r/5893/#review4891

I think this approach looks fine!
Attachment #8581922 - Flags: review?(cmanchester)
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 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
Attachment #8581922 - Flags: review?(cmanchester)
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.
Not sure why mozreview is showing those 'with marionette' changes in base.py, they aren't in my local commit.
Comment on attachment 8581922 [details]
MozReview Request: bz://987360/ahal

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

Excellent!
Attachment #8581922 - Flags: review?(cmanchester) → review+
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.
I accidentally broke xpcshell on b2g/android. Here's a bustage fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acbab9e22691
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
Attachment #8581922 - Flags: review+
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.
Attachment #8581922 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/7fb698f70e79
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Attached patch Enable chunk-by-runtime on try (obsolete) — Splinter Review
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.
Attachment #8578120 - Attachment is obsolete: true
Comment on attachment 8586073 [details] [diff] [review]
Enable chunk-by-runtime on try

Sorry, wrong bug..
Attachment #8586073 - Attachment is obsolete: true
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?
No, good point. I think support-files is also space delimited. I'll patch it up before it gets heavy use.
Attachment #8581922 - Attachment is obsolete: true
Attachment #8618102 - Flags: review+
See Also: → 1178441
You need to log in before you can comment on or make changes to this bug.