Closed Bug 1596567 Opened 5 years ago Closed 5 years ago

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

Categories

(Testing :: Reftest, defect, P2)

Version 3
defect

Tracking

(firefox74 fixed)

RESOLVED FIXED
mozilla74
Tracking Status
firefox74 --- fixed

People

(Reporter: ahal, Assigned: gbrown)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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.

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(ahal)
Flags: needinfo?(ahal)
Priority: -- → P2

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.

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

Blocks: 1577302
Assignee: nobody → 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.

:ahal do you have thoughts on comment 5 ^ ?

Flags: needinfo?(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.

Flags: needinfo?(ahal)

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

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.

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

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.

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"

Pushed by gbrown@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bd9fd617d3c9 Normalize test ids for reftests, crashtests, and jsreftests; r=ahal,jmaher
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(gbrown)

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

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

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.

Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: needinfo?(gbrown)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: