Closed
Bug 938712
Opened 7 years ago
Closed 7 years ago
Mach mochitest commands should be smart enough to pick the suite for you
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla33
People
(Reporter: ahal, Assigned: gps)
References
Details
(Whiteboard: [good first bug])
Attachments
(4 files)
3.30 KB,
patch
|
automatedtester
:
review+
|
Details | Diff | Splinter Review |
4.23 KB,
patch
|
automatedtester
:
review+
|
Details | Diff | Splinter Review |
2.53 KB,
patch
|
automatedtester
:
review+
|
Details | Diff | Splinter Review |
3.16 KB,
patch
|
automatedtester
:
review+
|
Details | Diff | Splinter Review |
There are a ton of mochitest-* mach commands. In some cases these commands are actually different suites (e.g mochitest-plain and mochitest-chrome), but many of these are actually just the same suite running on different platforms (e.g mochitest-plain, mochitest-remote and mochitest-b2g-desktop). In the latter case, the mochitest command could easily detect what build is currently active and automatically choose which entry path to pick based off of this. This would mean less clutter in the help, and no need to remember which command is meant for which platform. I don't know if this makes a good first bug as it could be a bit tricky, but if a contributor wants to tackle it, I'd be happy to mentor.
Assignee | ||
Comment 1•7 years ago
|
||
Somewhat related to bug 920193. There is also code in part 2 of bug 920849 that will make this much, much easier. Feel free to steal review from ted.
Updated•7 years ago
|
Assignee: nobody → anshu.avinash35
Assignee | ||
Comment 2•7 years ago
|
||
Now that mochitests are almost 100% in .ini manifests, the mach commands should be able to load up all-tests.json and glean the mochitest flavor from that metadata. There's even a Python API for that. See testing/xpcshell/mach_commands.py.
Reporter | ||
Comment 3•7 years ago
|
||
Hi Anshu, I noticed you are interested in this bug. Let me know if you have any questions or need a pointer in the right direction. (In reply to Gregory Szorc [:gps] from comment #2) > Now that mochitests are almost 100% in .ini manifests, the mach commands > should be able to load up all-tests.json and glean the mochitest flavor from > that metadata. That would only be possible if a test path is passed in right? So we'd still need separate targets for all the flavours (or at least an argument) in the event that they want to run all tests.
Assignee | ||
Comment 4•7 years ago
|
||
The end goal I've been working towards is to make the end-user's life as easy as possible. We do this by optimizing for common scenarios: 1) Run all tests relevant for a given feature 2) Run a specific test(s) with customizations For #1, we assume "run all tests in a directory [tree]" is a sufficient proxy for "feature." The solution here is `mach test path/to/directory` which will discover every relevant test and sequentially run each test suite for the subset of tests in that directory. This will enable new contributors to run all relevant tests without requiring them to know details about what the different test suites do. This is huge. This is the goal of bug 920193. For #2, I don't think we'll be able to shoehorn every test suite customization into `mach test`. So, we'll still need suite-specific mach commands (mach xpcshell-test, mach mochitest-plain, etc). Given that the mach mochitest-* commands all share similar flags, we should be able to consolidate down to |mach mochitest|. That's the purpose of this bug. We were previously blocked on not having mochitest flavor metadata available to mach nor the build config. (Yay for moz.build data leading to better UX!) I think it makes sense to provide a --flavor (bikeshed welcome) argument to limit tests to a certain mochitest flavor. If a path is not given, the entire suite is executed. |mach mochitest| without arguments would be ambiguous. Does that all make sense? Does that sound like a reasonable proposal?
Reporter | ||
Comment 5•7 years ago
|
||
Yeah, I was thinking the same thing. I was just asking how we specify the flavor. I think --flavor would be a good solution, though I think it would be nice if it had a default so |mach mochitest| without arguments ran mochitest-plain. So: mach mochitest # run all mochitest plain mach mochitest --flavor foo # run all mochitest foo mach mochitest path/to/tests # run all mochitests in path; autodetect flavor mach mochitest --flavor foo path/to/tests # error out, --flavor not necessary FTR, the original intent of this bug was to get rid of the platform specific commands, e.g mochitest-remote or mochitest-b2g-desktop. Though I agree we should continue from there into flavors.
Reporter | ||
Comment 6•7 years ago
|
||
Haven't seen anything from Anshu, resetting assignee.
Assignee: anshu.avinash35 → nobody
Updated•7 years ago
|
Whiteboard: [mentor=ahal] → [mentor=ahal][good first bug]
Updated•7 years ago
|
Mentor: ahalberstadt
Whiteboard: [mentor=ahal][good first bug] → [good first bug]
Assignee | ||
Comment 7•7 years ago
|
||
The MochitestOptions class has a class-local definition of the options going into the optparse instance. The default values for these options would be reused by subsequent consumers. In the case of the profile path, the same temporary directory would be used. In the case of list arguments, subsequent runs would inherit members added by earlier runs. This patch should make subsequent runs free from the baggage of the first.
Mentor: ahalberstadt
Attachment #8441717 -
Flags: review?(dburns)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → gps
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•7 years ago
|
||
The `mach mochitest` command is now implemented. Given test path arguments, it will identify mochitests of any flavor and run the appropriate mochitest suite. If tests from multiple suites are present, it will invoke each suite separately. Although, the output in this mode isn't very friendly. There are a number of enhancements that could be made to this command, including the abilities to filter by flavor and sub-suite. These will come in another patch.
Attachment #8441718 -
Flags: review?(dburns)
Assignee | ||
Comment 9•7 years ago
|
||
`mach mochitest` now accepts a --flavor argument to limit execution to tests of a certain flavor. Executing `mach mochitest --flavor=X` should be equivalent to executing `mach mochitest-X`. This paves the road to deprecating the various `mach mochitest-X` commands.
Attachment #8441722 -
Flags: review?(dburns)
Assignee | ||
Comment 10•7 years ago
|
||
`mach test` now dispatches through `mach mochitest --flavor` where supported. As part of testing this patch, it was discovered that `mach test` may have been broken for quite some time, as it was still referring to "test_file" arguments instead of "test_paths." This has been corrected.
Attachment #8441728 -
Flags: review?(dburns)
Reporter | ||
Comment 11•7 years ago
|
||
This is awesome.. though it doesn't get rid of the 'mochitest-remote' and 'mochitest-b2g-desktop' commands (which was my original motivation for filing this bug). If you don't feel like tackling that as well, I don't mind filing a new bug for it. Also, did you mean to flag :AutomatedTester for review?
Updated•7 years ago
|
Attachment #8441718 -
Flags: review?(dburns) → review+
Updated•7 years ago
|
Attachment #8441722 -
Flags: review?(dburns) → review+
Comment 12•7 years ago
|
||
Comment on attachment 8441717 [details] [diff] [review] Allow multiple instantiations of MochitestOptions Review of attachment 8441717 [details] [diff] [review]: ----------------------------------------------------------------- r+ after updating the comment ::: testing/mochitest/mochitest_options.py @@ +427,5 @@ > def __init__(self, **kwargs): > > optparse.OptionParser.__init__(self, **kwargs) > for option, value in self.mochitest_options: > + # Copy lists to avoid updating original, thus allowing multiple the comment here doesnt seem to agree with the code. It is overwriting value['default'].
Attachment #8441717 -
Flags: review?(dburns) → review+
Comment 13•7 years ago
|
||
Comment on attachment 8441728 [details] [diff] [review] Rewrote mach test mochitest suites via mach mochitest Review of attachment 8441728 [details] [diff] [review]: ----------------------------------------------------------------- lgtm Follow up needed for devtools to have a flavour when doing --subsuite as discussed.
Attachment #8441728 -
Flags: review?(dburns) → review+
Assignee | ||
Comment 14•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/cb5244b95c40 https://hg.mozilla.org/integration/fx-team/rev/9f6baa671811 https://hg.mozilla.org/integration/fx-team/rev/3dedef93c16d https://hg.mozilla.org/integration/fx-team/rev/5301f1aaf305
https://hg.mozilla.org/mozilla-central/rev/cb5244b95c40 https://hg.mozilla.org/mozilla-central/rev/9f6baa671811 https://hg.mozilla.org/mozilla-central/rev/3dedef93c16d https://hg.mozilla.org/mozilla-central/rev/5301f1aaf305
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Reporter | ||
Comment 16•7 years ago
|
||
This got resolved without merging the different mochitest platform commands. I filed bug 1046992 to fix that.
You need to log in
before you can comment on or make changes to this bug.
Description
•