mach try fuzzy <path> is ignoring skip-if and disabled in mochitest.ini
Categories
(Developer Infrastructure :: Try, defect, P2)
Tracking
(firefox-esr60 unaffected, firefox-esr68 wontfix, firefox68 wontfix, firefox69 wontfix, firefox70 fixed)
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox-esr68 | --- | wontfix |
firefox68 | --- | wontfix |
firefox69 | --- | wontfix |
firefox70 | --- | fixed |
People
(Reporter: bwc, Assigned: egao)
References
(Blocks 1 open bug, Regression)
Details
(Keywords: regression)
Attachments
(1 file)
This seems to be a fairly recent development, but I have not had time to bisect.
Comment 1•6 years ago
|
||
Selecting <paths> was actually completely broken up until recently. This was probably fallout from bug 1489100. I think what's happening is that since that bug, we pass the paths to the individual tests into the harnesses (rather than leaving it as the directory). And mochitest has a feature where if you specify a path to a test explicitly, it ignores the skip annotations in the manifest.
Maybe we can modify mochitest so it only ignores the skip annotations if you pass a single path to a test.
Reporter | ||
Comment 2•6 years ago
|
||
That seems like a reasonable approach.
Comment 3•6 years ago
|
||
Oh my mistake, looks like mochitest already only does this if there was a single test to run:
https://searchfox.org/mozilla-central/rev/44a212460990ffffecf50a8e972d3cbde2e7216b/testing/mochitest/runtests.py#1473
There must be something else going on here.
Updated•6 years ago
|
Comment 5•6 years ago
|
||
Can this be prioritized? It's causing a lot of wasted time for developers who don't realize what is happening, as skip-if conditions do not work as expected and mass-break tests (cf. bug 1552565).
Comment 6•6 years ago
|
||
I'm pretty swamped at the moment, maybe Joel can find someone to take a look?
Otherwise this is on my radar still. Feel free to ping me again after the all-hands if there hasn't been any movement.
Comment 7•6 years ago
|
||
:egao, I know you have a few new projects on your plate, this would help out with :Gijs' use case as well as retriggering custom test verify jobs, etc. :) Possibly you could get this onto your queue of bugs/projects in the new few weeks?
Assignee | ||
Comment 8•6 years ago
•
|
||
I don't mind looking at this, though I'd like a couple of use cases so I have something to test against, that would be nice.
The way I understand the problem, is given a mochitest manifest file, ./mach mochitest <manifest_file>
is ignoring all the skip-if
clauses. This doesn't cover the scenario when a specific test file is passed into the argument.
Does the above sound correct?
Comment 9•6 years ago
•
|
||
(In reply to Edwin Gao (:egao) from comment #8)
I don't mind looking at this, though I'd like a couple of use cases so I have something to test against, that would be nice.
The way I understand the problem, is given a mochitest manifest file,
./mach mochitest <manifest_file>
is ignoring all theskip-if
clauses. This doesn't cover the scenario when a specific test file is passed into the argument.Does the above sound correct?
No, when you run:
./mach mochitest path/to/directory/with/tests
locally, the skip-ifs are honoured (as they should be).
When you push to try, and use try-test-paths or pass a path to ./mach try fuzzy
, they're ignored, and that's the problem.
An easy test is pushing an artifact trypush with the browser/components/customizableui/
path and a single mochitest-browser-chrome job on macos. This currently fails because the skip-if at https://searchfox.org/mozilla-central/rev/4606c7974a68cab416c038acaedcae49eed93822/browser/components/customizableui/test/browser.ini#40-42 is ignored, running a test that can never pass on mac.
Does that help?
Assignee | ||
Comment 10•6 years ago
|
||
:gijs - yes, that helps, did not know you could pass a path into ./mach try fuzzy
. That gives me a starting point to investigate, thanks.
Comment 11•6 years ago
|
||
I think (based on previous discussions I've seen, but not being familiar with the code), that this code causes the paths passed to be expanded into individual files, which will expand
mach try fuzzy browser/components/customizableui/
into a task that runs something like
mach mochitest <...> browser/components/customizableui/browser_934113_menubar_removable.js <...>
the later of which won't honor the skip-if.
I think what needs to be done is figure out a way to split the paths passed to mach try fuzzy
to get split by test suite, without resolving them to individual file names.
Comment 12•6 years ago
|
||
I also would really like if this was fixed… with the all the churn and regressions in Try tooling it's become very frustrating to push to try recently. ./mach try chooser
doesn't even support paths and ./mach try syntax
(which does) is almost unusable since the trychooser website was decommissioned if you need to test on a platform that doesn't have an obvious platform name (e.g. for the android x86_64 emulator).
I guess this is a dupe of bug 1523717 though?
Comment 13•6 years ago
|
||
(In reply to Tom Prince [:tomprince] from comment #11)
I think (based on previous discussions I've seen, but not being familiar with the code), that this code causes the paths passed to be expanded into individual files, which will expand
mach try fuzzy browser/components/customizableui/
into a task that runs something like
mach mochitest <...> browser/components/customizableui/browser_934113_menubar_removable.js <...>
the later of which won't honor the skip-if.
I think what needs to be done is figure out a way to split the paths passed to
mach try fuzzy
to get split by test suite, without resolving them to individual file names.
I believe we previously just forwarded directories wholesale, but then this change was done in support of code coverage. Assuming the code coverage scheduler is something we want to work in the future (which I believe we will), then we might need to solve this on the harness side somehow.
Updated•6 years ago
|
Assignee | ||
Comment 14•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 16•5 years ago
|
||
Comment 17•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Updated•3 years ago
|
Updated•2 years ago
|
Description
•