Closed
Bug 1288352
Opened 8 years ago
Closed 8 years ago
Stylo-specific reftest.list
Categories
(Testing :: Reftest, defect, P3)
Testing
Reftest
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: shinglyu, Unassigned)
References
Details
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
Details |
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 | ||
Comment 1•8 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•8 years ago
|
||
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)
Updated•8 years ago
|
Version: Version 3 → Trunk
Comment 3•8 years ago
|
||
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)
Comment 4•8 years ago
|
||
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•8 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•8 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)
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
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)
Comment 9•8 years ago
|
||
Shing, do you reftest lists in bug 1288350 make this bug obsolete?
Depends on: 1288350
Flags: needinfo?(slyu)
Reporter | ||
Comment 10•8 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)
Comment 11•8 years ago
|
||
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
Updated•8 years ago
|
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•