975977 - reftest fails - skip-if(condition) does not skip reftest as expected

Status: RESOLVED WONTFIX

(Testing :: Reftest, defect)

Description

[Vincent Chen [:vichen]]

Attachment: reftest.js.diff

Story:
To skip xul loading test in B2G/OOP, we plan to add skip-if(B2G&&browserIsRemote) before the reftests with xul loading, but some skipped test still be tested.

For example: we plan to skip this one (http://dxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/reftest.list#1245)
fuzzy(255,15) == 472020-1a.xul 472020-1-ref.xul

and add skip-if in front of it

skip-if(B2G&&browserIsRemote) fuzzy(255,15) == 472020-1a.xul 472020-1-ref.xul

but ./mach reftest-remote still test this case.

Reason:
From reftests definition (In http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/README.txt#59), the failure-type can be multiple-defined, but from implementation (http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js#851) the skip failure type may be override by latter.

In above case: skip-if(B2G&&browserIsRemote) fuzzy(255,15) == 472020-1a.xul 472020-1-ref.xul
The fuzzy failure type will override skip-if and it makes skip-if no use.

Two potential solutions:
1. Make skip, skip-if two failure type override every other failture type.
2. Put the skip-if() in the latter part of failure type definitions, like:
fuzzy(255,15) skip-if(B2G&&browserIsRemote) == 472020-1a.xul 472020-1-ref.xul

Comment 1

[Vincent Chen [:vichen]]

> Two potential solutions:
> 1. Make skip, skip-if two failure type override every other failture type.
attachment 8380526 [details] [diff] [review] is potential solution for this solution.

dbaron,
Need your help to review the issue I found while using skip-if to skip some reftests under B2G/OOP. Please correct me if I miss or misunderstand reftests. If my understand is correct, please advise me to go solution 1 or 2. Or, please advise someone else I can consult for this kind of entry-level question.

Comment 2

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

(In reply to Vincent Chen [:vichen] from comment #0)
> 2. Put the skip-if() in the latter part of failure type definitions, like:
> fuzzy(255,15) skip-if(B2G&&browserIsRemote) == 472020-1a.xul 472020-1-ref.xul

I prefer this one.

It is intentional that the later annotations override the earlier ones.

Comment 3

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

(Though, alternatively, we could change to using Math.max() in more places, but I'd prefer not to add the separate skip variable.  If we changed the behavior, though, I'd like to know what existing manifests we were changing the meaning of.)

Comment 4

[Vincent Chen [:vichen]]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #3)
> (Though, alternatively, we could change to using Math.max() in more places,
> but I'd prefer not to add the separate skip variable.  If we changed the
> behavior, though, I'd like to know what existing manifests we were changing
> the meaning of.)

From http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js#130 we define:
const EXPECTED_DEATH = 3;
const EXPECTED_FUZZY = 4;

And, this make it is no use even if we use Math.max() in more spaces. Just curious, why EXPECTED_FUZZY is higher priority than EXPECTED_DEATH (skip)?

I'll keep current behavior and move skip-if annotation to former part of annotations to skip xul loading reftests.


Another thing, I do reftests again and found we have totally 10,563 tests and only 4921 being tested in B2G (non-oop) in current mainfest (data from reftests output). This is not good as compared to my previous summary by using ( >6500 tests and about 800 is skipped which is only 11% being skipped).

Comment 5

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

(In reply to Vincent Chen [:vichen] from comment #4)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #3)
> > (Though, alternatively, we could change to using Math.max() in more places,
> > but I'd prefer not to add the separate skip variable.  If we changed the
> > behavior, though, I'd like to know what existing manifests we were changing
> > the meaning of.)
> 
> From
> http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.
> js#130 we define:
> const EXPECTED_DEATH = 3;
> const EXPECTED_FUZZY = 4;
> 
> And, this make it is no use even if we use Math.max() in more spaces. Just
> curious, why EXPECTED_FUZZY is higher priority than EXPECTED_DEATH (skip)?

Yes, if we wanted to use Math.max() more, we would need to fix that.  Right now it's not a problem, though, since fuzzy doesn't make sense to use at a per-directory level, and now the Math.max() is only used to merge the directory-level annotations (those on he "include" line) with the file-level ones (on the ==, !=, or load line).

> Another thing, I do reftests again and found we have totally 10,563 tests
> and only 4921 being tested in B2G (non-oop) in current mainfest (data from
> reftests output). This is not good as compared to my previous summary by
> using ( >6500 tests and about 800 is skipped which is only 11% being
> skipped).

Large numbers of reftest *directories* were marked as skip-if(B2G).  At the time I thought it was because we didn't have enough testing hardware to run the full suite, but now I've heard that teams are welcome to enable any of their own tests that pass.  So working on removing all of these skip-if(B2G) annotations would be great.

Comment 6

[Vincent Chen [:vichen]]

Follow comment 2.