Shing Lyu [:shinglyu] Assignee
	Description • 8 years ago

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

	Shing Lyu [:shinglyu] 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.

	Xidorn Quan [:xidorn] UTC+11
	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.

	Shing Lyu [:shinglyu] 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.

	Xidorn Quan [:xidorn] UTC+11
	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.

	David Baron :dbaron:
	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

	Pulsebot
	Comment 7 • 8 years ago

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

	Pulsebot
	Comment 8 • 8 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/55ebdfc93471
Backed out changeset c7df34767efb for stylo test failures

	Carsten Book [:Tomcat]
	Comment 9 • 8 years ago

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

	Shing Lyu [:shinglyu] 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

	Shing Lyu [:shinglyu] Assignee
	Comment 11 • 8 years ago

Re-landing this since Bug 1349130 has landed.

	Pulsebot
	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

	Iris Hsiao [:ihsiao]
	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

	Pulsebot
	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

	David Baron :dbaron:
	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.

	Shing Lyu [:shinglyu] Assignee
	Comment 16 • 8 years ago

Thanks, I'll update the patch accordingly.

	Shing Lyu [:shinglyu] 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.)

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

	Shing Lyu [:shinglyu] 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?

	Cameron McCormack (:heycam)
	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.)

	David Baron :dbaron:
	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).

	Pulsebot
	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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	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

	Pulsebot
	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

	Carsten Book [:Tomcat]
	Comment 26 • 8 years ago
bugherder

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