1596567 - Normalize 'crashtest' and 'jsreftest' test identifiers in ActiveData

Status: RESOLVED FIXED

(Testing :: Reftest, defect, P2)

Description

[Andrew Halberstadt [:ahal]]

According to ActiveData there are 42188088 unique jsreftests and 14957312 unique crashtests in the past week alone.

This is obviously False. It looks like things like IP addresses, ports and worker paths are making their way into the test identifiers which means we are probably introducing an entirely new set of tests each time they run.

I assume we need to fix this by updating what gets added to the structuredlog in the reftest harness.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:ahal, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Geoff Brown [:gbrown]]

In common local usage, relative test paths are translated to file urls with absolute paths by resolveManifests:

https://searchfox.org/mozilla-central/rev/690e903ef689a4eca335b96bd903580394864a1c/layout/tools/reftest/runreftest.py#587

reftest.jsm and manifest.jsm seem to exclusively use those file urls.

runreftest.py already stores topsrcdir; that might be useful.

Comment 3

[Geoff Brown [:gbrown]]

In CI, different reftest tasks log suite_start from different locations:

• reftest.jsm, (including desktop crashtests, android crashtests, android reftests)
• runreftest.py (including desktop reftests)

https://searchfox.org/mozilla-central/rev/6bceafe72cad5d5752667b4b6bd595d3a4047ca3/layout/tools/reftest/reftest.jsm#535
https://searchfox.org/mozilla-central/rev/6bceafe72cad5d5752667b4b6bd595d3a4047ca3/layout/tools/reftest/runreftest.py#940

Comment 4

[Geoff Brown [:gbrown]]

I think this is promising, but certainly needs refinement:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5b081a4f6a72ad78a49c6966256e1c45fa099bf

Comment 5

[Geoff Brown [:gbrown]]

This is (relatively) simple and produces ideal results for all CI tasks:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11f0b3fc6b170a1d1d389f8cd21540db9a354a59

but it doesn't work for runs which specify manifests or tests (most local runs, and a possibility for future CI runs?).

The strategy is to construct test.identifier from the test urls, then trim the urls:

• for jsreftests, take everything to the left of "test="
• for crashtests and reftests, note the location of the top manifest, crashtests.list or reftest.list, compare to the known location of the top manifests to infer the topsrcdir; then trim the urls for the identifier by removing everything to the left of the topsrcdir.

Comment 6

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

:ahal do you have thoughts on comment 5 ^ ?

Comment 7

[Andrew Halberstadt [:ahal]]

I'd prefer if we didn't have to rely on regexes and stuff.. Do you know what the format of the tests are that we pass into the JS harness from the Python side? Maybe we could make sure those are relative paths to the root and then save them somewhere before we start turning them into file urls and the like. Or if that's not possible, maybe we could pass both the relative paths and the URL formats from Python?

I guess this seems simple enough, but it will definitely need to work with runs that specify manifests in the (very) near future.

Comment 8

[Kyle Lahnakoski [:ekyle]]

:gbrown I must agree with ahal: I have tried a couple of times to parse the test names, but they continue to change

Comment 9

[Geoff Brown [:gbrown]]

https://searchfox.org/mozilla-central/rev/94a19efaaf1eefa58e36047f73b49ed0c5162163/layout/tools/reftest/runreftest.py#214

In python, resolveManifests() uses findManifests which calls absManifestPath and manifestURL to translate any relative paths to file: urls of absolute paths. Those urls are passed to reftest.jsm and manifest.jsm via the reftest.manifests pref; in reftest.jsm (manifest.jsm), all tests are urls, and test identifiers are derived from the urls.

https://searchfox.org/mozilla-central/rev/94a19efaaf1eefa58e36047f73b49ed0c5162163/layout/tools/reftest/runreftest.py#867
https://searchfox.org/mozilla-central/rev/94a19efaaf1eefa58e36047f73b49ed0c5162163/layout/tools/reftest/manifest.jsm#725

For Android, the python side is over-ridden to construct an http: url instead:

https://searchfox.org/mozilla-central/rev/94a19efaaf1eefa58e36047f73b49ed0c5162163/layout/tools/reftest/remotereftest.py#46

In python, preserving relative paths, or generating relative paths alongside absolute ones is relatively easy. We should be mindful of passing relative file paths between processes -- maybe okay for the identifiers, but I think we shouldn't rely on that for the test urls. A further complication is that, for some control paths, test urls are generated in manifest.jsm -- when including manifests, for instance. I'm working through that now.

Also, jsreftests are a little different:
https://searchfox.org/mozilla-central/rev/94a19efaaf1eefa58e36047f73b49ed0c5162163/js/src/tests/lib/manifest.py#249

And we need to keep Windows os.sep complications in mind.

Bright ideas and full solutions are welcome, but I'm not necessarily looking for collaboration here -- just thinking out loud.

Comment 10

[Geoff Brown [:gbrown]]

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

In python, preserving relative paths, or generating relative paths alongside absolute ones is relatively easy.

Yes, but...we don't just want relative paths; we want a relative path that is consistent regardless of how reftests have been run, or where.

Example, from Linux CI runs:
"file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/writing-mode/1079154-1-vertical-rl-columns.html"

I think of this as "layout/reftests/writing-mode/1079154-1-vertical-rl-columns.html" -- relative to topsrcdir. topsrcdir ought to be available on all CI platforms, and for local runs. But how to determine topsrcdir? In CI, runreftests.py is unpacked to (Linux) /builds/worker/workspace/build and run from there, so os.getcwd() is no help; nor is the normal SCRIPT_DIRECTORY hack. build_obj is not available in CI. runreftest.py has an existing topsrcdir option, but it is mach only; maybe I'll expand that...

Comment 11

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

if we can make it relative to topsrcdir that would be best- is there a way to generate extra data from the build system when we generate the reftest.tar.zg file as an artifact? I assume there are many ways to solve this.

Comment 12

[Geoff Brown [:gbrown]]

Good signs here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3908207168da39543bfbadd5db79b4b8731bafb9

Comment 13

[Geoff Brown [:gbrown]]

Attachment: Bug 1596567 - Normalize test ids for reftests, crashtests, and jsreftests; r=

Use test identifiers that look like posix-compliant relative file paths rather than URLs; identifier
"paths" are relative to topsrcdir -- an intuitive, brief mapping from identifier to test file.
Instead of using a test identifier like:
"file:///builds/worker/workspace/build/tests/reftest/tests/dom/svg/crashtests/398926-fill.svg" ->
"dom/svg/crashtests/398926-fill.svg"
A jsreftest example:
"file:///builds/worker/workspace/build/tests/jsreftest/tests/js/src/tests/jsreftest.html?test=test262/built-ins/Array/prototype/indexOf/15.4.4.14-9-b-i-31.js" ->
"js/src/tests/test262/built-ins/Array/prototype/indexOf/15.4.4.14-9-b-i-31.js"

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd9fd617d3c9
Normalize test ids for reftests, crashtests, and jsreftests; r=ahal,jmaher

Comment 15

[Stefan Hindli [:stefan_hindli]]

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

Comment 16

[Marco Castelluccio [:marco]]

From https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=be5b500761e1af396026f540749600f3d8e1cb4f&searchStr=reftest&selectedJob=287036087, there are still absolute paths (e.g. "file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/w3c-css/submitted/conditional3/reftest.list" in https://firefoxci.taskcluster-artifacts.net/UgRYwqdzRYSqf72A1nxGgQ/0/public/test_info//reftest_errorsummary.log).

Comment 17

[Marco Castelluccio [:marco]]

It would also be nice if we could normalize the slashes in the paths, so the same manifest isn't considered as two different manifests on Linux and Windows just because of the / vs \\.

Comment 18

[Geoff Brown [:gbrown]]

This bug normalized the test identifiers used in the suite_start record, only. That seems to be working as intended, and definitely includes transformation of path separators.

I was unaware that the errorsummary.log was also of interest. Will have a look....

Comment 19

[Geoff Brown [:gbrown]]

Actually, the issue is not which log (errorsummary vs raw log) but the data type. This bug addressed test identifiers - not manifest names.

I think the same strategy can be applied to manifests. Let's handle that as a separate bug.