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

Status: RESOLVED FIXED

(Testing :: Reftest, enhancement, P3)

Description

[Shing Lyu [:shinglyu]]

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 1

[Shing Lyu [:shinglyu]]

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

Review commit: https://reviewboard.mozilla.org/r/116976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116976/

Comment 2

[Shing Lyu [:shinglyu]]

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.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 4

[Shing Lyu [:shinglyu]]

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.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 6

[David Baron :dbaron:]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7df34767efb
Make reftest run stylo-vs-gecko test without special reftest.list. r=dbaron

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55ebdfc93471
Backed out changeset c7df34767efb for stylo test failures

Comment 9

[Carsten Book [:Tomcat]]

backed out for test failures like  https://treeherder.mozilla.org/logviewer.html#?job_id=84598506&repo=autoland and others

Comment 10

[Shing Lyu [:shinglyu]]

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

Comment 11

[Shing Lyu [:shinglyu]]

Re-landing this since Bug 1349130 has landed.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1debf80d996
Make reftest run stylo-vs-gecko test without special reftest.list. r=dbaron

Comment 13

[Iris Hsiao [:ihsiao]]

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

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82106bfb9fe8
Backed out changeset c1debf80d996 for stylo test failures

Comment 15

[David Baron :dbaron:]

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.

Comment 16

[Shing Lyu [:shinglyu]]

Thanks, I'll update the patch accordingly.

Comment 17

[Shing Lyu [:shinglyu]]

Comment on attachment 8843196 [details]
Bug 1344132 - Make reftest run stylo-vs-gecko test without special reftest.list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116976/diff/1-2/

Comment 18

[Shing Lyu [:shinglyu]]

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.)

Comment 19

[David Baron :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?

Comment 20

[Shing Lyu [:shinglyu]]

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?

Comment 21

[Cameron McCormack (:heycam)]

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.)

Comment 22

[David Baron :dbaron:]

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).

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f41e4a4bc4
Make reftest run stylo-vs-gecko test without special reftest.list. r=heycam

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/148e6b4cbd7e
Make reftest run stylo-vs-gecko test without special reftest.list. r=heycam

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/148e6b4cbd7e