1580914 - |mach test-info| doesn't yield complete data

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Andrew Halberstadt [:ahal]]

Brian noticed that the fission-test-info artifacts don't match up with the spreadsheets listing the skipped tests manually. From his e-mail:

I started on this per-component graph and realized the numbers I’m seeing from the artifacts don’t line up with what I see on the spreadsheet which is tracking individual tests (https://docs.google.com/spreadsheets/d/1kjp32JTuB4axM3wKx0iIYL2Ado-HcyeBhoY-siGxYAs/edit#gid=1560718888).

For example, on that spreadsheet I see 600+ rows with not too many crossed out. But if I load the artifact from yesterday (https://index.taskcluster.net/v1/task/gecko.v2.mozilla-central.pushdate.2019.09.11.latest.source.test-info-fission/artifacts/public/test-info-fission.json) I’m seeing only 198 skipped tests and 27 failed tests.

Any ideas what’s going on with the discrepancy here? Is the artifact collecting incomplete data, is the spreadsheet out of date, or am I just misreading it?

To test this out, I ran ./mach test-info both with a build and without and noticed the results are different. I believe this is happening because certain moz.build files are only read based on build conditions. E.g:
https://searchfox.org/mozilla-central/source/toolkit/toolkit.mozbuild#182

So if any of those directories define test manifests, the test resolver (which mach test-info depends on) won't pick them up. So the "simple" solution would be to get the test-info-fission tasks to run configure before running ./mach test-info. But there are two downsides here:

A) It will need to use the build image, making it more expensive and slower to run.
B) Even with a buildconfig, it might be missing manifests. E.g, here is another condition:
https://searchfox.org/mozilla-central/source/toolkit/themes/moz.build#21

So even if we do run ./configure we'll never consider every moz.build file, which means we might be missing out on manifests.

Luckily, the build system has something called "file-system mode". This essentially means that instead of following the DIRS variables, moz.build traversal will instead simply look for moz.build files in the file-system. This mode is designed to pick-up every moz.build file regardless of build config for cases like this.

So I propose we add a file_system_traversal=False flag to the test resolver:
https://searchfox.org/mozilla-central/source/testing/mozbase/moztest/moztest/resolve.py

And make sure ./mach test-info uses it. I can look into switching the test-info-fission tasks over to the build image in the meantime.

Comment 1

[Andrew Halberstadt [:ahal]]

I ran into some problems trying to get the test-info-fission task running configure. Luckily Geoff said he was planning to take a look at this pretty soon.

I have a little bit of context around how moz.build reading works which might be useful..

First, the test resolver calls into this function which instantiates a BuildReader class and calls read_topsrcdir. This read_topsrcdir function starts at the root moz.build and calls reader.read_mozbuild() on it.

Now this read_mozbuild function will follow the DIRS variable to find child moz.builds by default, but this behaviour can be disabled by setting descend=False. The reader also has a convenience all_mozbuild_paths function.

So.. I think you can do something like:

1. Pass some sort of file_system_traversal=True into gen_test_backend and if it's set then..
2. Iterate over all_mozbuild_paths calling read_mozbuild(descend=False) on each (I'm not sure if order is important here.. will need testing or build peer feedback.
3. Make sure we re-create the same output format as the read_topsrcdir function.

Then we would process every moz.build file.

p.s I could have sworn that reader.py had some way of running a file-system traversal more conveniently but I can't seem to find it. Either it was moved/removed or I'm misremembering.

Comment 2

[Andrew Halberstadt [:ahal]]

It's also worth noting that even with all of the above we still might be missing data. That's because even if we read all moz.build files, the *_MANIFEST keys themselves could be hidden behind build configuration, like this:
https://searchfox.org/mozilla-central/source/dom/media/moz.build#88

I don't know how many instances like that there are.. but if we want to make sure we hit 100% of the tests no matter what, we'll need to parse the moz.build AST. Luckily there is already precedent for this using the SPHINX_TREES variables:
https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/reader.py#909

That function even has a comment about how we should make it more generic so we can use it for other variables as well. So maybe it's worth looking into. If we go this route, I think the steps in comment 1 wouldn't be necessary.

Comment 3

[Andrew Halberstadt [:ahal]]

(In reply to Andrew Halberstadt [:ahal] from comment #2)

It's also worth noting that even with all of the above we still might be missing data. That's because even if we read all moz.build files, the *_MANIFEST keys themselves could be hidden behind build configuration, like this:
https://searchfox.org/mozilla-central/source/dom/media/moz.build#88

I think a very strong argument could be made that we shouldn't be using build metadata to filter out tests, and we should use test manifests + mozinfo instead. So a valid solution here might be to remove the manifests from these conditions and make sure they have appropriate skip-ifs instead.

p.s. I just love how that directory has both a test and tests subdir.. and apparently tests means webrtc is enabled.

Comment 4

[Geoff Brown [:gbrown]]

(In reply to Andrew Halberstadt [:ahal] from comment #1)
Thanks much for the pointers.

So.. I think you can do something like:

1. Pass some sort of file_system_traversal=True into gen_test_backend and if it's set then..
2. Iterate over all_mozbuild_paths calling read_mozbuild(descend=False) on each (I'm not sure if order is important here.. will need testing or build peer feedback.

When I try this, I hit

["NameError: name 'GeckoSharedLibrary' is not defined\n"]

when I call read_mozbuild(descend=False) on accessible/interfaces/ia2/moz.build. I suppose we need to read_mozbuild in correct order -- an order different from iterating all_mozbuild_paths. Thoughts?

Comment 5

[Andrew Halberstadt [:ahal]]

Yeah, I'm pretty sure we need to make sure "parent" moz.builds need to be read before "child" moz.builds. So in this case, the moz.build files need to be read in this order:

moz.build
accessible/moz.build
accessible/interfaces/moz.build
accessible/interfaces/ia2/moz.build

There's an algorithm that already yields moz.builds in this order in the find_relevant_mozbuilds function:
https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/reader.py#1243

Though not sure if you can repurpose that for your use case or not. Note, it would be helpful if you changed all_mozbuild_paths to yield them in an order that doesn't break things.

Comment 6

[Andrew Halberstadt [:ahal]]

One naive algorithm that should work would be:

yield all paths with 0 path delimiters
yield all paths with 1 path delimiter
yield all paths with 2 path delimiters
...

Comment 7

[Geoff Brown [:gbrown]]

(In reply to Andrew Halberstadt [:ahal] from comment #0)

Brian noticed that the fission-test-info artifacts don't match up with the spreadsheets listing the skipped tests manually. From his e-mail:

I started on this per-component graph and realized the numbers I’m seeing from the artifacts don’t line up with what I see on the spreadsheet which is tracking individual tests (https://docs.google.com/spreadsheets/d/1kjp32JTuB4axM3wKx0iIYL2Ado-HcyeBhoY-siGxYAs/edit#gid=1560718888).

For example, on that spreadsheet I see 600+ rows with not too many crossed out. But if I load the artifact from yesterday (https://index.taskcluster.net/v1/task/gecko.v2.mozilla-central.pushdate.2019.09.11.latest.source.test-info-fission/artifacts/public/test-info-fission.json) I’m seeing only 198 skipped tests and 27 failed tests.

Taking a step back, we should note that there are multiple factors at play:

• the spreadsheet rows include rows for each component name (eg "Core::Audio/Video: Playback")
• the spreadsheet rows include rows for now-passing tests (eg. dom/ipc/tests/JSWindowActor/browser_destroy_callbacks.js)
• there are some errors in the spreadsheet (eg. dom/ipc/tests/browser_crash_oopiframe.js is actually skip-if=!fission)
• some skipped tests appear in local test-info reports from a full build context but not in the fission task (eg. toolkit/content/tests/browser/browser_autoplay_videoDocument.js, browser/base/content/test/trackingUI/browser_trackingUI_cookies_subview.js and 11 other tracking UI tests)
• probably other cases

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Geoff Brown [:gbrown] from comment #7)

(In reply to Andrew Halberstadt [:ahal] from comment #0)

Brian noticed that the fission-test-info artifacts don't match up with the spreadsheets listing the skipped tests manually. From his e-mail:

I started on this per-component graph and realized the numbers I’m seeing from the artifacts don’t line up with what I see on the spreadsheet which is tracking individual tests (https://docs.google.com/spreadsheets/d/1kjp32JTuB4axM3wKx0iIYL2Ado-HcyeBhoY-siGxYAs/edit#gid=1560718888).

For example, on that spreadsheet I see 600+ rows with not too many crossed out. But if I load the artifact from yesterday (https://index.taskcluster.net/v1/task/gecko.v2.mozilla-central.pushdate.2019.09.11.latest.source.test-info-fission/artifacts/public/test-info-fission.json) I’m seeing only 198 skipped tests and 27 failed tests.

Taking a step back, we should note that there are multiple factors at play:

• the spreadsheet rows include rows for each component name (eg "Core::Audio/Video: Playback")
• the spreadsheet rows include rows for now-passing tests (eg. dom/ipc/tests/JSWindowActor/browser_destroy_callbacks.js)
• there are some errors in the spreadsheet (eg. dom/ipc/tests/browser_crash_oopiframe.js is actually skip-if=!fission)
• some skipped tests appear in local test-info reports from a full build context but not in the fission task (eg. toolkit/content/tests/browser/browser_autoplay_videoDocument.js, browser/base/content/test/trackingUI/browser_trackingUI_cookies_subview.js and 11 other tracking UI tests)
• probably other cases

Good points. This spreadsheet might be a better point of comparison: https://docs.google.com/spreadsheets/d/1sodDcyeuCuT_SX0Zqg_27wti7hO8dodLVsgMJKtx-l4/edit. :kmag is in charge of updating that but I'm not sure how he's doing it. Perhaps he could explain what he's using for this.

Comment 9

[Kris Maglione [:kmag]]

I run test-info locally, from a Linux tree with a full build. I've been using an old version of the test-info patch which doesn't handle !fission correctly. I haven't updated to my new version because my plan has been to just switch to using the artifacts from treeherder when they're ready, and it didn't seem worth making other changes in the meantime.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Kris Maglione [:kmag] from comment #9)

I run test-info locally, from a Linux tree with a full build. I've been using an old version of the test-info patch which doesn't handle !fission correctly. I haven't updated to my new version because my plan has been to just switch to using the artifacts from treeherder when they're ready, and it didn't seem worth making other changes in the meantime.

OK, so comparing with the sheet in Comment 8 isn't helpful then, since it's using the same data source as the artifacts AIUI.

We can definitely come up with a better count from https://docs.google.com/spreadsheets/d/1kjp32JTuB4axM3wKx0iIYL2Ado-HcyeBhoY-siGxYAs/edit#gid=1560718888 to account for the points in Comment 7, but the numbers were far enough apart that it seems like it's surfacing a real issue.

Comment 11

[Andrew Halberstadt [:ahal]]

(In reply to Brian Grinstead [:bgrins] from comment #10)

OK, so comparing with the sheet in Comment 8 isn't helpful then, since it's using the same data source as the artifacts AIUI.

Not quite. Kris probably had a build config when running ./mach test-info whereas the task doesn't, which likely accounts for the difference. This bug is about making ./mach test-info not depend on a build config.

Edit: I may have misread what you meant.. in that case carry on :).

Comment 12

[Kris Maglione [:kmag]]

I suspect at this point the difference is just that the numbers from automation still don't come from a complete build. I initially tried to do my local runs from a separate checkout without a build and got similar sorts of results.

Comment 13

[Geoff Brown [:gbrown]]

(In reply to Geoff Brown [:gbrown] from comment #7)

• some skipped tests appear in local test-info reports from a full build context but not in the fission task (eg. toolkit/content/tests/browser/browser_autoplay_videoDocument.js, browser/base/content/test/trackingUI/browser_trackingUI_cookies_subview.js and 11 other tracking UI tests)

The primary cause of discrepancies is that MOZ_BUILD_APP is not defined in the fission task environment, excluding browser/ notably:

https://searchfox.org/mozilla-central/rev/4218cb868d8deed13e902718ba2595d85e12b86b/moz.build#160-164

The easiest way to get MOZ_BUILD_APP defined is to run configure. Locally, if I run 'mach clobber', the truncated results seen in the fission task are reproduced; after 'mach configure' I get the full results (same as with a full build, currently 543 fission-excluded tests).

Comment 14

[Geoff Brown [:gbrown]]

Attachment: Bug 1580914 - Ensure config environment before generating test-info report; r=

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b742509f8342
Ensure config environment before generating test-info report; r=ahal

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b742509f8342