1288352 - Stylo-specific reftest.list

Status: RESOLVED FIXED

(Testing :: Reftest, defect, P3)

Description

[Shing Lyu [:shinglyu]]

Check-in a version of stylo-reftest.list that contains known passing reftests for stylo

Future contributors should at least run this test suite when checking in stylo-related code. 

If they add new properties, they should run the full `./mach reftest` and add the new passing tests to the stylo-reftest.list suite.

Comment 1

[Shing Lyu [:shinglyu]]

I already have a draft patch, which contains 

   4218 REFTEST TEST-PASS 
   3413 REFTEST TEST-UNEXPECTED-FAIL 
    101 REFTEST TEST-KNOWN-FAIL 
     41 TEST-UNEXPECTED-FAIL 
     41 REFTEST PROCESS-CRASH 
      8 REFTEST TEST-UNEXPECTED-PASS 

Roughly 4218 passing tests. There are 41 crashes that prevents us from running the full suite. I'll check-in after cleaning up.

Comment 2

[Shing Lyu [:shinglyu]]

Attachment: Bug 1288352 - Creating stylo-specific reftest list for stylo testing.

Review commit: https://reviewboard.mozilla.org/r/66796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66796/

Comment 3

[Cameron McCormack (:heycam)]

I'm concerned that these stylo-reftest.list files will quickly become out of sync with the main reftest.list files.  As other (non-stylo) contributors make changes to reftest.list files, it's likely they won't update stylo-reftest.list at the same time.

What do you think about an approach where we instead use the existing fails-if(...) and skip-if(...) syntax to mark test that we run (and know we still fail) or need to skip due to crashing?  So we could have:

  fails-if(stylo) a.html a-ref.html

for individual tests.  Using |fails-if(...) include blah/reftest.list| may not work (though skip-if(...) should).  Maybe we can make that work?

Comment 4

[Cameron McCormack (:heycam)]

One advantage to using fails-if() is that it will be clear when we start passing a test that we need to remove the fails-if().  A disadvantage I suppose is that having most tests marked as fails-if() will mean that it takes a long time to run the reftests locally, since we're not just running the ~4000 that we know we pass.

Comment 5

[Shing Lyu [:shinglyu]]

Will that be checked into m-c? I'm afraid that people will get pissed off if we put the fails-if() flag all over the place.

Comment 6

[Shing Lyu [:shinglyu]]

Ah, I believe there won't be a sync issue. The stylo-reftest.list is for people to quickly check for regressions. When they implement new properties that is suppose to make more tests pass, they should run the full suite (the original reftest.list) and add the new passing tests to stylo-reftest.list. 

I think when we reach ~80% passing we can switch to the fails-if() approach. Right now it will just pollute all the reftest.list files.

Comment 7

[Cameron McCormack (:heycam)]

OK, after the meeting this morning I understand it's a temporary situation until we get real CI running (which is hopefully soon), so having the duplicated reftest.list manifests with just the tests we pass for the moment should be fine.  It probably doesn't need to be checked into mozilla-central though, so we can just have it in the gecko-dev stylo branch.

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8774276 [details]
Bug 1288352 - Creating stylo-specific reftest list for stylo testing.

https://reviewboard.mozilla.org/r/66796/#review63868

Looks good in general.

There are some files named reftest-stylo.list added in the patch.  Should they be removed?

These new files aren't referenced from anywhere:

* layout/reftests/reftest-sanity/stylo-urlprefixtests.list
* layout/reftests/border-radius/reftest-test.list
* layout/reftests/position-dynamic-changes/horizontal/stylo-reftest_plain.list

I'm not sure where the changes to layout/reftests/position-dynamic-changes/horizontal/reftest_plain.list came from.

r=me to land this in the gecko-dev stylo branch with these things fixed (or explained)

Comment 9

[Chris Peterson [:cpeterson]]

Shing, do you reftest lists in bug 1288350 make this bug obsolete?

Comment 10

[Shing Lyu [:shinglyu]]

Strictly speaking, NO. But in practice I think the one in Bug 1288350 is more useful right now. So I would suggest merging Bug 1288350 and leave this one out. When we reach, say ~80% compatibility with the old Gecko style system, we can re-consider this one and maybe update the patch.

Comment 11

[Chris Peterson [:cpeterson]]

P3 because we will want to revisit this bug before we ship, now that Cameron has landed Shing's reftest support in bug 1288350.