58 bytes, text/x-review-board-request
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.
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.
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/
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?
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.
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.
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.
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 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)
Shing, do you reftest lists in bug 1288350 make this bug obsolete?
Depends on: 1288350
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.
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
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.