Closed Bug 1541424 Opened 7 months ago Closed 3 months ago

mach try fuzzy <path> is ignoring skip-if and disabled in mochitest.ini

Categories

(Firefox Build System :: Try, defect, P2)

defect

Tracking

(firefox-esr60 unaffected, firefox-esr68 wontfix, firefox68 wontfix, firefox69 wontfix, firefox70 fixed)

RESOLVED FIXED
mozilla70
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)

See https://treeherder.mozilla.org/#/jobs?repo=try&revision=838c426723b9d26f19fd181f92b34563ba37b0ef&selectedJob=237726271

This seems to be a fairly recent development, but I have not had time to bisect.

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.

Regressed by: 1489100

That seems like a reasonable approach.

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.

Priority: -- → P2
Duplicate of this bug: 1544426

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).

Flags: needinfo?(ahal)

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.

Flags: needinfo?(ahal) → needinfo?(jmaher)

: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?

Flags: needinfo?(jmaher) → needinfo?(egao)

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?

Flags: needinfo?(egao)

(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 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?

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?

: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.

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 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?

(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.

Attachment #9079530 - Attachment description: Bug 1541424 - vary the expansion of test path depending on mach try subcommand selected → Bug 1541424 - vary the expansion of test path behavior depending on mach try subcommand
Attachment #9079530 - Attachment description: Bug 1541424 - vary the expansion of test path behavior depending on mach try subcommand → Bug 1541424 - vary the behavior of test path resolve in mach try
Duplicate of this bug: 1523717
Assignee: nobody → egao
Attachment #9079530 - Attachment description: Bug 1541424 - vary the behavior of test path resolve in mach try → Bug 1541424 - ensure path expansion in mach try respects manifest annotations
Pushed by egao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ccd3e9f16a2
ensure path expansion in mach try respects manifest annotations r=ahal,marco
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.