Closed Bug 1756878 Opened 2 years ago Closed 1 year ago

Add a linting rule for mochitest.ini / browser.ini / chrome.ini files to ensure tests are in alphabetical order

Categories

(Developer Infrastructure :: Lint and Formatting, task)

Tracking

(firefox111 fixed)

RESOLVED FIXED
Tracking Status
firefox111 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The --start-at and --end-at arguments to ./mach mochitest are really handy for running subsets of a series of tests.

I tried to use them on these tests today:

https://searchfox.org/mozilla-central/rev/152a28cc2a64a8efefba2160550e14987b943b74/toolkit/content/tests/widgets/mochitest.ini#36-38

I used:

./mach mochitest toolkit/content/tests/widgets/ --start-at test_videocontrols_vtt.html --end-at test_videocontrols_size.html

The problem is that these tests are not in alphabetical order, but internally, it seems that the mochitest test runner sorts the tests in alphabetical order before letting me select a subsection. So after alphabetizing the tests, it hit this code to try to use my start-at and end-at arguments: https://searchfox.org/mozilla-central/rev/152a28cc2a64a8efefba2160550e14987b943b74/testing/mochitest/chunkifyTests.js#26

and since test_videocontrols_vtt.html is after test_videocontrols_size.html when in alphabetical order, it determined that my start index was after my end index, and so it ended up using something like:

tests.slice(15, 12)

which always returns an empty array when the start index is greater than the end index.

In general, I think it makes sense to have our mochitests be in alphabetical order... it generally just makes it easier to find the tests when there are many of them. If that's what our test harness expects too, it might make sense to add a linting rule to enforce their being in alphabetical order.

I suspect that switching up the order of tests will introduce a whole bunch of failures / new intermittents. Ideally tests don't depend on artifacts from previous tests, but in practice they often do. Would probably make more sense to ensure that the harness keeps --start-at and --end-at in the order that they are defined in the manifest.

I suspect that switching up the order of tests will introduce a whole bunch of failures / new intermittents.

Will it? Because I believe the test runner already runs them in alphabetical order. This is more about making the mochitest manifests reflect their order of execution.

That would be news to me, but I've been shocked at what I found in mochitest before :p.. Maybe it's just the implementaiton of --start-at and --end-at that does this? Sounds like some further investigation is required here.

Seems to be the case even if I don't use --start-at or --end-at.

STR (this just does some chrome mochitests since it's not the actual tests that we care about)

  1. Open mozilla-central/toolkit/content/tests/widgets/chrome.ini, and note the order of the listed tests. In particular, notice that test_contextmenu_nested.xhtml is listed first.
  2. Run the chrome mochitests but keep the window open afterwards, like this:
./mach mochitest --flavor=chrome toolkit/content/tests/widgets/ --keep-open
  1. Note the tests as listed in the test runner UI

ER:

Ideally, the tests would run in the same order that they're in in the chrome.ini file

AR:

They're run in alphabetical order. test_contextmenu_menugroup.xhtml is run first.

I just ran into this again in another directory where I was trying to narrow an intermittent down that only ran when running the whole test directory. I had to manually order the items in the browser.ini file in order to correctly use --start-at and --end-at.

ahal, would it be okay if I took a crack at this?

Flags: needinfo?(ahal)

Hm, I think I would prefer we fix the harness to run them in the order that they're defined.. though if they have been running in alphabetical order this whole time, then that change might be the one that causes all kinds of new intermittents in CI.

Joel, curious if you have any thoughts here.

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

I am 99.9% sure they run alphabetical. With that said many .ini files are not alphabetical- I would prefer if we fixed that so visually looking at the manifest it would flow like execution does. The summary of this bug for a linting rules makes sense.

Is there a way to prove tests (xpcshell (runs in parallel), marionette, mochitest*) run alphabetical?

Flags: needinfo?(jmaher)

I did some debugging and afaict the tests don't get sorted on the Python side.. so it must be the JS harness that is sorting them. This means that the various flavours could have differing behaviour. One easy way to test is find a manifest for the given flavour that isn't alphabetized, then use the test path filter in Treeherder to find a task that runs it. Then search for the test paths and see if they ran in order or not.

Mike, given mochitests have apparently been running alphabetically this whole time and I somehow didn't know that, I think going ahead with this lint rule makes sense. It still feels like a harness bug that they don't run in manifest order, but fixing that will likely be harder than standing up this lint rule (due to possible test interdependencies).

Hey ahal, I've got a WIP up in https://phabricator.services.mozilla.com/D141264. I've been reading this wonderful documentation but I haven't set up any of the taskcluster or docs stuff yet. Does it look like I'm on the right track though?

Flags: needinfo?(ahal)

Hey, sorry for the delay.. yes this looks great! I added a couple comments. Docs probably aren't super necessary in this case, though a quick test would be appreciated!

Flags: needinfo?(ahal)

It looks like globs for include directives was removed intentionally in https://hg.mozilla.org/mozilla-central/rev/a80cb43e8d5e (bug 1448417). Do you think this linter is a good reason to reintroduce it, so we can match on **/mochitest.ini, **/chrome.ini, **/browser.ini?

Flags: needinfo?(ahal)

To be honest, probably not. The logic to support globs in both include and exclude was very complicated and I don't think I ever had it to a point where all edge cases were solved. It was a big maintenance pain.

We do have a few linters for test manifests, and they tend to start with all .ini files and then add exclusions:
https://searchfox.org/mozilla-central/source/tools/lint/test-manifest-skip-if.yml#15

Though this case is different since we only want to lint some test manifests.. We could exclude all the manifest files we don't want (e.g, **/xpcshell.ini). New manifests that don't conform to these exclusions would get caught up in it.. But is enforcing alphabetized tests such a bad thing in general?

If we did want to support this more properly, I'd be more inclined to adding a filterfn key that could point to a Python function that takes a path as input and returns True if it should be processed otherwise False. Though this does open up the possibility of long running filter functions hurting performance.

Flags: needinfo?(ahal)
Product: Firefox Build System → Developer Infrastructure

Hello! I am here via https://phabricator.services.mozilla.com/D166738#5499012 , and seeing as I was already considering what we could do to fix the problem in a more structural fashion, I am excited to learn most of the work is already done!

Mike, is this revivable, maybe for now just by forcing a filter for mochitest.ini, browser.ini and chrome.ini (and maybe a11y.ini) inside the python script itself rather than the linter config? We could still additionally try to add some exclude rules for perf's sake, but that'd guarantee that we don't need to find everything to exclude, necessarily?

(I also think we could actually do this for xpcshell because AIUI those tests are normally run in parallel and with separate processes, so it shouldn't matter (famous last words) but we could deal with that in a follow-up bug...)

Flags: needinfo?(mconley)
Assignee: nobody → mconley
Attachment #9268123 - Attachment description: WIP: Bug 1756878 - Add a linter to enforce alphabetical ordering for mochitest manifests. r?ahal! → Bug 1756878 - Add a linter to enforce alphabetical ordering for mochitest manifests. r?ahal!
Status: NEW → ASSIGNED

Gonna clear needinfo given there's an up-to-date patch now :-)

Flags: needinfo?(mconley)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee8acb03bbbf
Add a linter to enforce alphabetical ordering for mochitest manifests. r=ahal
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Blocks: 1815447
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: