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

Status: RESOLVED FIXED

(Developer Infrastructure :: Try, defect, P2)

Description

[Byron Campen [:bwc]]

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.

Comment 1

[Andrew Halberstadt [:ahal]]

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.

Comment 2

[Byron Campen [:bwc]]

That seems like a reasonable approach.

Comment 3

[Andrew Halberstadt [:ahal]]

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.

Comment 5

[:Gijs (he/him)]

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

[Andrew Halberstadt [: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.

Comment 7

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

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

Comment 8

[Edwin Takahashi (:egao | infrequent contributor)]

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

[:Gijs (he/him)]

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

Comment 10

[Edwin Takahashi (:egao | infrequent contributor)]

: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

[Tom Prince [:tomprince]]

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

[Matthew N. [:MattN]]

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

[Andrew Halberstadt [:ahal]]

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

Comment 14

[Edwin Takahashi (:egao | infrequent contributor)]

Attachment: Bug 1541424 - ensure path expansion in mach try respects manifest annotations

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ccd3e9f16a2
ensure path expansion in mach try respects manifest annotations r=ahal,marco

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/3ccd3e9f16a2