Normalize 'crashtest' and 'jsreftest' test identifiers in ActiveData
Categories
(Testing :: Reftest, defect, P2)
Tracking
(firefox74 fixed)
Tracking | Status | |
---|---|---|
firefox74 | --- | fixed |
People
(Reporter: ahal, Assigned: gbrown)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
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•5 years ago
|
||
The priority flag is not set for this bug.
:ahal, could you have a look please?
For more information, please visit auto_nag documentation.
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
In common local usage, relative test paths are translated to file urls with absolute paths by resolveManifests:
reftest.jsm and manifest.jsm seem to exclusively use those file urls.
runreftest.py already stores topsrcdir; that might be useful.
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Comment 3•5 years ago
|
||
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
Assignee | ||
Comment 4•5 years ago
|
||
I think this is promising, but certainly needs refinement:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5b081a4f6a72ad78a49c6966256e1c45fa099bf
Assignee | ||
Comment 5•5 years ago
|
||
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.
Reporter | ||
Comment 7•5 years ago
|
||
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•5 years ago
|
||
:gbrown I must agree with ahal: I have tried a couple of times to parse the test names, but they continue to change
Assignee | ||
Comment 9•5 years ago
|
||
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:
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.
Assignee | ||
Comment 10•5 years ago
|
||
(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•5 years ago
|
||
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.
Assignee | ||
Comment 12•5 years ago
|
||
Assignee | ||
Comment 13•5 years ago
|
||
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•5 years ago
|
||
Comment 15•5 years ago
|
||
bugherder |
Comment 16•5 years ago
|
||
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).
Updated•5 years ago
|
Comment 17•5 years ago
|
||
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 \\
.
Assignee | ||
Comment 18•5 years ago
|
||
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....
Assignee | ||
Comment 19•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Description
•