Stylo-specific reftest.list

RESOLVED FIXED

Status

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: shinglyu, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Blocks: 1243581
(Reporter)

Comment 1

2 years ago
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.
(Reporter)

Comment 2

2 years ago
Created attachment 8774276 [details]
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/
Attachment #8774276 - Flags: review?(cam)
Version: Version 3 → Trunk
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?
Flags: needinfo?(slyu)
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.
(Reporter)

Comment 5

2 years ago
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.
Flags: needinfo?(slyu)
(Reporter)

Comment 6

2 years ago
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.
Flags: needinfo?(cam)
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.
Flags: needinfo?(cam)
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)
Attachment #8774276 - Flags: review?(cam)
Shing, do you reftest lists in bug 1288350 make this bug obsolete?
Depends on: 1288350
Flags: needinfo?(slyu)
(Reporter)

Comment 10

2 years ago
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.
Flags: needinfo?(slyu)
P3 because we will want to revisit this bug before we ship, now that Cameron has landed Shing's reftest support in bug 1288350.
Priority: -- → P3
Blocks: 1330412
No longer blocks: 1243581
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.