Closed Bug 1344132 Opened 4 years ago Closed 4 years ago

stylo: Make reftest run stylo-vs-gecko test without special reftest.list

Categories

(Testing :: Reftest, enhancement, P3)

Version 3
enhancement

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))
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: nobody → slyu
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)
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)
Attachment #8843196 - Flags: review?(xidorn+moz) → review?(dbaron)
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 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
backed out for test failures like  https://treeherder.mozilla.org/logviewer.html#?job_id=84598506&repo=autoland and others
Flags: needinfo?(slyu)
Depends on: 1349130
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)
Re-landing this since Bug 1349130 has landed.
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
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82106bfb9fe8
Backed out changeset c1debf80d996 for stylo test failures
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
Depends on: 1353643
Depends on: 1354404
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.
Thanks, I'll update the patch accordingly.
Flags: needinfo?(slyu)
Blocks: 1356988
No longer blocks: stylo
No longer blocks: 1356988
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)
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)
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)
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)
Attachment #8843196 - Flags: review?(dbaron) → review+
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)
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
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
https://hg.mozilla.org/mozilla-central/rev/148e6b4cbd7e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(shing.lyu)
You need to log in before you can comment on or make changes to this bug.