Closed Bug 1394603 Opened 2 years ago Closed 2 years ago

Clean up fails-if(!styloVsGecko) reftests by resetting to fails

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: jryans, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Stylo])

Attachments

(3 files)

`fails-if(!styloVsGecko)` effectively means the reftest fails on all style systems, but S vs. G passes since they both fail the same way.

We should reset these annotations to `fails` and changes S vs. G mode to ignore them.
Assignee: nobody → cpeterson
Here is a Try run that is mostly green, except for a bug found in my change to layout/reftests/scoped-style/reftest.list that I fixed and commented before submitting my review request here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=216049533799e309bbc159e3576dc219bdc711e0
Comment on attachment 8904157 [details]
Bug 1394603 - Replace fails-if(!styloVsGecko) to fails.

https://reviewboard.mozilla.org/r/175932/#review181338

Thanks for working on this, looks great overall! :)

See my notes about potential tweaks to the bug annotations.

::: dom/tests/reftest/xml-stylesheet/reftest.list:6
(Diff revision 1)
>  == css_relative_href.xml pass.svg
>  HTTP == css_relative_href_also_external.xml pass.svg
>  HTTP == css_relative_href_also_external_override.xml pass.svg
>  == embedded_dtd_id.svg pass.svg
>  != error_no_href.svg fail.svg
> -fails-if(!styloVsGecko) == lreas_selflink_dtd_id.svg pass.svg
> +fails == lreas_selflink_dtd_id.svg pass.svg # bug 631575

I believe in most cases, an annotation like "bug NNN" is expected to mean an open bug on file to fix the problem, but this bug is a resolved bug that had marked this test as failing.

Annotating the bug that marked something as failing less useful (since it can be recovered from VCS history), but if you'd like to keep it since you did the work to look it up, maybe expand the annotation to "Fails after bug NNN"?

::: layout/reftests/border-radius/reftest.list:40
(Diff revision 1)
>  # Test that radii too long are reduced
>  == border-reduce-height.html border-reduce-height-ref.html
>  skip-if(!webrender) pref(layers.advanced.border-layers,1) == border-reduce-height.html border-reduce-height-ref.html
>  
>  # Tests for border clipping
> -fails-if(!styloVsGecko) == clipping-1.html clipping-1-ref.html # background color should completely fill box; bug 466572
> +fails == clipping-1.html clipping-1-ref.html # background color should completely fill box; bug 466572 and bug 458131

Another bug annotation we may want to clarify.

::: layout/reftests/css-charset/reftest.list:6
(Diff revision 1)
>  == test-attribute.html pass.html
>  == test-charset-quotes.html pass.html
>  == test-charset-leading-space.html pass.html
>  == test-charset-trailing-space.html pass.html
>  == test-charset-utf-16-le-no-bom.html pass.html
> -fails-if(!styloVsGecko) == test-charset-utf-16-le-bom.html pass.html
> +fails == test-charset-utf-16-le-bom.html pass.html # bug 462458

Another bug annotation we may want to clarify.

::: layout/reftests/css-charset/reftest.list:9
(Diff revision 1)
>  == test-charset-trailing-space.html pass.html
>  == test-charset-utf-16-le-no-bom.html pass.html
> -fails-if(!styloVsGecko) == test-charset-utf-16-le-bom.html pass.html
> +fails == test-charset-utf-16-le-bom.html pass.html # bug 462458
>  == test-charset-utf-16-bom-le.html pass.html
>  == test-charset-utf-16-be-no-bom.html pass.html
> -fails-if(!styloVsGecko) == test-charset-utf-16-be-bom.html pass.html
> +fails == test-charset-utf-16-be-bom.html pass.html # bug 462458

Another bug annotation we may want to clarify.

::: layout/reftests/css-grid/reftest.list:3
(Diff revision 1)
>  default-preferences pref(layout.css.grid.enabled,true)
>  
> -fails-if(!styloVsGecko) == grid-whitespace-handling-1a.xhtml grid-whitespace-handling-1-ref.xhtml
> +fails == grid-whitespace-handling-1a.xhtml grid-whitespace-handling-1-ref.xhtml # bug 1000376

Another bug annotation we may want to clarify.

::: layout/reftests/view-source/reftest.list:1
(Diff revision 1)
> -fails-if(!styloVsGecko) random-if(styloVsGecko) == view-source-image.html view-source-image-ref.html # security checks prevent loading view-source:
> +fails == view-source-image.html view-source-image-ref.html # bug 989688: security checks prevent loading view-source:

Another bug annotation we may want to clarify.
Attachment #8904157 - Flags: review?(jryans) → review+
Comment on attachment 8904158 [details]
Bug 1394603 - Replace random-if(styloVsGecko) with skip-if.

https://reviewboard.mozilla.org/r/175934/#review181388

It would be nice to know why they are random...  :emilio suggested floating point differences between engines as one possibility.

In any case, I believe this makes enough sense.  We'll still have the skip-ifs in case we want to investigate further.
Attachment #8904158 - Flags: review?(jryans) → review+
Comment on attachment 8904159 [details]
Bug 1394603 - Infer asserts-if(styloVsGecko, X*2) from asserts(X).

https://reviewboard.mozilla.org/r/175936/#review181390

Thanks, this seems reasonable to me!
Attachment #8904159 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> Annotating the bug that marked something as failing less useful (since it
> can be recovered from VCS history), but if you'd like to keep it since you
> did the work to look it up, maybe expand the annotation to "Fails after bug
> NNN"?

OK. I removed some of my new bug comments that point to ancient bugs and added "fails after bug NNN" for references to recent Stylo bugs.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> It would be nice to know why they are random...  :emilio suggested floating
> point differences between engines as one possibility.

OK. I added comments explaining why we skip on these for styloVsGecko.
I made the recommended changes, fixed a merge conflict, and ran another Try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f31941d18867d2430e4f247d0730ff8fe9696886
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7957bf375853 -d 4f8769251cec: rebasing 418198:7957bf375853 "Bug 1394603 - Replace fails-if(!styloVsGecko) to fails. r=jryans"
merging layout/reftests/forms/progress/reftest.list
rebasing 418199:86b0afe1fbbc "Bug 1394603 - Replace random-if(styloVsGecko) with skip-if. r=jryans"
merging layout/reftests/forms/progress/reftest.list
warning: conflicts while merging layout/reftests/forms/progress/reftest.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7957bf375853 -d 867b17852af8: rebasing 418233:7957bf375853 "Bug 1394603 - Replace fails-if(!styloVsGecko) to fails. r=jryans"
merging layout/reftests/forms/progress/reftest.list
merging layout/reftests/writing-mode/reftest.list
rebasing 418234:86b0afe1fbbc "Bug 1394603 - Replace random-if(styloVsGecko) with skip-if. r=jryans"
merging layout/reftests/forms/progress/reftest.list
warning: conflicts while merging layout/reftests/forms/progress/reftest.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20551d68f602
Replace fails-if(!styloVsGecko) to fails. r=jryans
https://hg.mozilla.org/integration/autoland/rev/31088c59d895
Replace random-if(styloVsGecko) with skip-if. r=jryans
https://hg.mozilla.org/integration/autoland/rev/44117208f321
Infer asserts-if(styloVsGecko, X*2) from asserts(X). r=jryans
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fcf98211c151
Replace fails-if(!styloVsGecko) to fails. r=jryans
https://hg.mozilla.org/integration/autoland/rev/ffbae2a9f132
Replace random-if(styloVsGecko) with skip-if. r=jryans
https://hg.mozilla.org/integration/autoland/rev/026c2d5a3ee8
Infer asserts-if(styloVsGecko, X*2) from asserts(X). r=jryans
Too late for 56. Mass won't fix for 56.
You need to log in before you can comment on or make changes to this bug.