Closed
Bug 1344132
Opened 8 years ago
Closed 8 years ago
stylo: Make reftest run stylo-vs-gecko test without special reftest.list
Categories
(Testing :: Reftest, enhancement, P3)
Tracking
(firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: shinglyu, Assigned: shinglyu)
References
Details
Attachments
(1 file)
Make the reftest runner run stylo-vs-gecko style test when the --setpref=reftest.compareStyloToGecko=true flag is set. This will: 1) force the test expectation to be "==" 2) run the test file twice, one in stylo one in gecko (ignore the reference file) This will make us ready to run stylo-specific reftest using the origional reftest.list (with skip-if(stylo))
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
Oh sorry, it's not complete. I just realize that we may need to ignore all "skip", "fail", etc. flags if it's not specific for stylo.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → slyu
Comment 3•8 years ago
|
||
Comment on attachment 8843196 [details] Bug 1344132 - Make reftest run stylo-vs-gecko test without special reftest.list. Canceling since you said it is not complete.
Attachment #8843196 -
Flags: review?(xidorn+moz)
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8843196 [details] Bug 1344132 - Make reftest run stylo-vs-gecko test without special reftest.list. Based on the discussion in bug 1346692, we follow the "last-one-wins" rule. So no special handling needed for this patch, we can proceed with the review. Thank you.
Attachment #8843196 -
Flags: review?(xidorn+moz)
Updated•8 years ago
|
Attachment #8843196 -
Flags: review?(xidorn+moz) → review?(dbaron)
Comment 5•8 years ago
|
||
Comment on attachment 8843196 [details] Bug 1344132 - Make reftest run stylo-vs-gecko test without special reftest.list. It looks good to me, but strictly speaking I'm not a peer for reftest harness, so probably hand to dbaron.
Attachment #8843196 -
Flags: feedback+
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8843196 [details] Bug 1344132 - Make reftest run stylo-vs-gecko test without special reftest.list. https://reviewboard.mozilla.org/r/116976/#review123006
Attachment #8843196 -
Flags: review?(dbaron) → review+
Pushed by slyu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c7df34767efb Make reftest run stylo-vs-gecko test without special reftest.list. r=dbaron
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/55ebdfc93471 Backed out changeset c7df34767efb for stylo test failures
Comment 9•8 years ago
|
||
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=84598506&repo=autoland and others
Flags: needinfo?(slyu)
Assignee | ||
Comment 10•8 years ago
|
||
So I re-run the tests [0] and looks like Rs5 and Rs6 failures are intermittents. Rs1 and Rs12 unexpected passes are actually bugs in the reftest-stylo.list. I tried to fix them in Bug 1349130, after that landed we should be able to land this cleanly. [0] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d2163e67aee2d210f40d98fef2d2bbd79b073d8
Flags: needinfo?(slyu)
Comment 12•8 years ago
|
||
Pushed by slyu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c1debf80d996 Make reftest run stylo-vs-gecko test without special reftest.list. r=dbaron
Comment 13•8 years ago
|
||
sorry had to back this out for stylo test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=87184994&repo=autoland&lineNumber=6117 https://treeherder.mozilla.org/logviewer.html#?job_id=87185011&repo=autoland&lineNumber=4354 https://treeherder.mozilla.org/logviewer.html#?job_id=87185000&repo=autoland&lineNumber=2296 https://treeherder.mozilla.org/logviewer.html#?job_id=87184995&repo=autoland&lineNumber=6670 https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c1debf80d996cb10739512c9138c7f5954dd8d9f&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-searchStr=linux%20x64%20stylo
Flags: needinfo?(slyu)
Comment 14•8 years ago
|
||
Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/82106bfb9fe8 Backed out changeset c1debf80d996 for stylo test failures
Updated•8 years ago
|
Priority: -- → P3
Summary: Make reftest run stylo-vs-gecko test without special reftest.list → stylo: Make reftest run stylo-vs-gecko test without special reftest.list
Comment 15•8 years ago
|
||
One other thing that occurs to me is that we should probably have a special test manifest condition to indicate that this special thing is happening. Also, to fix the issue we mentioned today, you should also add the line: refPrefSettings = testPrefSettings; in the same gCompareStyloToGecko condition.
Updated•8 years ago
|
Updated•8 years ago
|
Blocks: stylo-nightly-build
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8843196 [details] Bug 1344132 - Make reftest run stylo-vs-gecko test without special reftest.list. Simply overwrite the ref-pref with test-pref will also override the css.servo.enabled flag set differently in each, so I choose to modify the function that adds preferences to the preference data structure. I didn't quite get the "special test manifest condition" part. To see if this is turned on, you can see that css.servo.enabled flag is being flipped in the reftest log. Also we should probably handle the "foo.html#id" kind of test. I was thinking about adding a "diable-stylo-to-gecko" (or maybe a shorter name) flag to reftest to revert back to traditional test method. But I can't get it to work stably. So I'll open a followup to fix that later, to avoid blocking Bug 1351548. (There are only 6 such tests, so it's probably OK for a short period of time.)
Attachment #8843196 -
Flags: review+ → review?(dbaron)
Comment 19•8 years ago
|
||
So my concern about the special test manifest condition is this: At some point we're going to want to run reftests the normal way with stylo. That will be the long-term approach. But when we prepare to start doing that, we're likely going to need to annotate reftest manifests differently for: 1. the special stylo-vs-gecko thing 2. running reftests normally with stylo I was thinking a "stylo" condition in the reftest manifest should be for (2), and there should be a special condition for (1), since in the medium term we're going to want a "stylo" condition to mean (1) rather than (2), since that's what conditions normally mean. On the other hand, maybe we'll switch to the normal way at the same time we enable stylo, which will be the same time that we can remove all stylo conditions entirely. But I suspect that may be too much change to try to land at once -- we'll probably need to be able to annotate manifests in stylo-specific ways for the normal way of running reftests. (For example, we might enable stylo on nightly, but then disable it on aurora/beta/release that branches off.) So what I was suggesting is that you add a styloVsGecko condition to the reftest manifest sandbox when doing this stylo-vs-gecko testing, and use that condition when doing manifest annotation for that setup. (Although I also worry that the necessary manifest annotation for that will be a *huge* patch.) What do you think?
Flags: needinfo?(slyu)
Assignee | ||
Comment 20•8 years ago
|
||
If we look at the milestones, it would look like this: * Now: Action: mark "skip" and "fail" for (1), mark some (2) (1) Most reftests, no flag (2) 5~6 tests with "#id", use "no-stylo" flag * When we get Stylo webcompat enough to run (2), but still want to run some (1): Action: mark "no-stylo" for (2) (1): half, no flag (2): half, use "no-stylo" flag * When Stylo is "ready": Action: Modify reftest runner to run only (2); Remove all the "no-stylo" flag (1): None, mark as fail or skip (2): Most, no flag If we use the inverse flag styloVsGecko: * Now: Action: mark "skip" and "fail" for stylo; Modify reftest runner to accept "styloVsGecko" flag; add "styloVsGecko" flag to almost every test (1) Most reftests, use "styloVsGecko" (2) 5~6 tests with "#id", no flag * When we get Stylo webcompat enough to run (2), but still want to run some (1): Action: Remove "styloVsGecko" flag for (1) (1): half, no flag (2): half, use "no-stylo" flag * When Stylo is "ready": Action: Remove all the "styloVsGecko" flag (1): None, mark as fail or skip (2): Most, no flag If we choose "no-stylo" flag, we need a big patch at stage 1 and stage 3, but it's less noisy in the reftest.list. If we choose the "styloVsGecko" flag, we need a big patch now and incrementally remove the flags. So if people will not be mad at "styloVsGecko" flag all over the place, I'm totally fine with it. Do you think it's OK to add "styloVsGecko" flag in all the reftest? Can we do it in the "include foo/reftest.list" level?
Flags: needinfo?(slyu) → needinfo?(dbaron)
Comment 21•8 years ago
|
||
Per offline discussion it sounds like we will just go with the current approach of keeping the "stylo" condition mean "compare Stylo to Gecko" and to worry about its change in meaning (or removal altogether) at the point we decide to start running Stylo versus Stylo reftests. (From my side, I don't think it's any worse to deal with that change later rather than now, and since we have everything ready to land with the current approach, better to get this in the tree now rather than later.)
Flags: needinfo?(dbaron)
Updated•8 years ago
|
Attachment #8843196 -
Flags: review?(dbaron) → review+
Comment 22•8 years ago
|
||
So I wasn't aiming for something you'd have to add everywhere. What I still don't understand is what you're going to do with a test that says: fails == foo.html foo-ref.html Aren't you going to need to change almost everything like that to: fails-if(!stylo) == foo.html foo-ref.html since foo.html in Gecko will actually be equal to foo.html in stylo? Do you have a way to avoid having to annotate all the tests like that? Or are you planning to annotate all of them? If you're planning to annotate all of them, I'd rather it say fails-if(!styloVsGecko) than fails-if(!stylo).
Flags: needinfo?(slyu)
Comment 23•8 years ago
|
||
Pushed by slyu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f41e4a4bc4 Make reftest run stylo-vs-gecko test without special reftest.list. r=heycam
![]() |
||
Comment 24•8 years ago
|
||
Backed out bug 1344132 and bug 1351548 for failing reftest reftest-opaque-layer-fail.html and many reftest failures for Stylo builds: Bug 1344132 https://hg.mozilla.org/integration/mozilla-inbound/rev/1d3a3ae57b06ae9ef71cf670727ee099aa1e248c Bug 1351548 https://hg.mozilla.org/integration/mozilla-inbound/rev/6232431e65ef49c045f822c4c1c1335f9ff6d2a9 https://hg.mozilla.org/integration/mozilla-inbound/rev/f75640e4a0cc3deb4cc7d7b1d1aa544916ad5c60 https://hg.mozilla.org/integration/mozilla-inbound/rev/e4b8f7c680ad1d88d2eeefb2ad63c656b435fc8e https://hg.mozilla.org/integration/mozilla-inbound/rev/1f9d0f8e65b91a5fee031950fb3940be44381194#l142.144 has > fails fails-if(stylo) As far as I know, the "fails-if" will overwrite the "fails". For stylo, there are more assertions than expected. Please also check the other reftest failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=aebb0ceeb20ae6031f456f5893db0ba4ec96d9e9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Comment 25•8 years ago
|
||
Pushed by slyu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/148e6b4cbd7e Make reftest run stylo-vs-gecko test without special reftest.list. r=heycam
Comment 26•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/148e6b4cbd7e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(shing.lyu)
You need to log in
before you can comment on or make changes to this bug.
Description
•