473817 - allow conditional 'include' in reftest manifests

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[(not currently active) Ted Mielczarek]

Currently you can only conditionally skip (or fail) individual tests. In bug 473577 I'm writing reftests for a test plugin. I'd like to be able to skip them all if the test plugin isn't present (or we're built without plugin support or something). Currently I'd have to have a fails-if for every line in the manifest, which seems ugly. It'd be nice to be able to skip including the whole manifest if we don't have plugin support (or don't have the test plugin, whatever).

Comment 1

[Sylvain Pasche]

Or maybe use a manifest global condition instead? The drawback of having conditional includes is that it creates a dependency between the including and the included manifests. So if you run the conditionally included manifest separately, you won't get the same results as if you run it from the including manifest.

I think it could be useful to let the current include mechanism only be a way of discovering all the available manifests. It means a reftest implementation is free to ignore the include statements and crawl the source tree looking for all the reftest.list files (that's what the reftest importer from the browsertests project does actually).

Comment 2

[(not currently active) Ted Mielczarek]

That's not a bad idea. I don't know what it would look like, but some sort of manifest-global fail/skip/etc might be good.

Comment 3

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

Attachment: skip-if(cond) include manifest.list (1.0)

simple patch to allow skip-if(cond) so we can ignore an include directive.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 517439 [details] [diff] [review]
skip-if(cond) include manifest.list (1.0)

If expected_status == EXPECTED_DEATH, this absolutely needs to report a REFTEST-KNOWN-FAIL and increment gTestResults.Skip.  Really, it should do that for each test in the manifest, but I'm ok with leaving that for later as long as you add a FIXME that really, instead of skipping the manifest, we should propagate all the conditions (not just skip) into the manifest (with sensible priority for overriding).

Comment 5

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

Attachment: skip-if(cond) include manifest.list (2.0)

thanks for the previous review.  This patch is simple as well, but propagates the expected result to the include file and applies it.  So for the case of android where we have skip-if(Android), we will see the same total number of tests, just a few more will be in the known problems area.

Comment 6

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

can we get a review on this?  this is needed to turn on reftests for mobile/android.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 521273 [details] [diff] [review]
skip-if(cond) include manifest.list (2.0)

>+        if (inherited_status != EXPECTED_PASS) {
>+            expected_status = inherited_status;
>+        }

This is the one bit I don't like here:  I think we probably do care about the listed status.

What I'd suggest instead is that here, you do:

expected_status = Math.max(expected_status, inherited_status);

And then add a comment above the line "const EXPECTED_PASS = 0;" saying:

// The order of these constants matters, since when we have a status
// listed for a *manifest*, we combine the status with the status for
// the test by using the *larger*.  
// FIXME: In the future, we may also want to use this rule for combining
// statuses that are on the same line (rather than making the last one
// win).

Finally, could you add to layout/tools/reftest/README.txt by changing the line:

   include <relative_path>

to:

   <failure-type>* include <relative_path>

   <failure-type> is the same as listed below for a test item.  As for 
   test items, multiple failure types listed on the same line are 
   combined by using the last matching failure type listed.  However, 
   the failure type on a manifest is combined with the failure type on 
   the test (or on a nested manifest) with the rule that the last in the
   following list wins:  fails, random, skip.  (In other words, skip 
   always wins, and random beats fails.)

r=dbaron with that

Comment 8

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

Attachment: skip-if(cond) include manifest.list (2.1)

updated patch, going to do a sanity run on try server before pushing.

Comment 9

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

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=c8f8da9431cb