1617261 - Disallow `skip-if(...) include` in reftest manifests

Status: NEW

(Testing :: Reftest, task, P3)

Description

[Andrew Halberstadt [:ahal]]

Now that bug 1616368 is fixed, it's easy to apply a skip-if to all tests in a manifest. This means, we no longer need to rely on the skip-if(condition) include manifest idiom to skip entire manifests.

In that other bug, :dbaron suggested I remove all skip-if includes and make it illegal to use. The benefit of using default to handle the skip-if is that we'll have accurate skip info even if running the child manifest directly.

E.g, if parent/reftest.list has a skip-if(true) include child/reftest.list in it, then that skip-if wouldn't be picked up if I ran ./mach reftest parnet/child/reftest.list directly.

Comment 1

[Andrew Halberstadt [:ahal]]

Also worth mentioning that due to scheduling expectations, it will be a requirement that we don't use skip-if in the root reftest manifest. So preventing this outright will also prevent accidentally adding one there.

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1617261 - Replace all 'skip-if(..) include' annotations from reftest/crashtest manifests with defaults

This ensures that the 'skip-if' still applies even when running the skipped manifest directly.

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1617261 - [reftest] Disallow 'skip-if(..) include' in reftest/crashtests manifests

Depends on D63722

Comment 4

[Andrew Halberstadt [:ahal]]

These patches cause some issues:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fea89d1798d266b2e78a20a3e238bb0f7d3f0951

They expose an issue in the reftest manifest parser. Basically the test:

skip-if(true) fails == foo.html bar.html

will run despite the skip-if(true). This is because fails is overwriting expected_status from EXPECTED_DEATH to EXPECTED_FAIL here:
https://searchfox.org/mozilla-central/source/layout/tools/reftest/manifest.jsm#226

This was never an issue before because it would be silly to specify both skip-if and fails on the same test (thinking about it maybe not that silly). But now with defaults, we can get into this situation with:

defaults skip-if(true)
== foo.html bar.html
fails foo.html bar2.html

I think the solution here is to have skip-if not use expected_status. After all, "skip" isn't really a result status. We could skip a test that is expected to pass or we could skip a test that is expected to fail. The result status should be independent from the skip status.

Comment 5

[Andrew Halberstadt [:ahal]]

I think fuzzy has this same problem:

fuzzy(0-1,0-2) fails == foo.html bar.html

Here, fails is going to overwrite EXPECTED_FUZZY with EXPECTED_FAIL. Which means that the test could theoretically pass with fuzz, but we wouldn't flag it with TEST-UNEXPECTED-PASS because we would be ignoring the fuzz.

Similarly:

fails fuzzy(0-1,0-2) == foo.html bar.html

will clobber EXPECTED_FAIL with EXPECTED_FUZZY.

Comment 6

[Andrew Halberstadt [:ahal]]

The fuzzy fails convention seems to be widely used in the wild:
https://searchfox.org/mozilla-central/source/layout/reftests/border-dotted/reftest.list#8

In otherwords.. those tests could be passing with fuzz with webrender, but we have no way of knowing. Since fixing the fuzzy part could cause many TEST-UNEXPECTED-PASS results, I think I'm going to focus on skip-if here, and file a follow-up for fuzzy.