Closed Bug 1362255 Opened 3 years ago Closed 3 years ago

Make w3c-css/import-tests.py import from wpt rather than csswg-test

Categories

(Testing :: Reftest, enhancement, P3)

enhancement

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: lochang)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

csswg-test is now deprecated and no longer updates. We should make import-tests.py use wpt repo rather than csswg-test so that we can continue importing new tests before we eventually switch to wpt.
Assignee: nobody → xidorn+moz
There's a bunch of stuff in this patch that looks like it's fixes for a different patch.
Flags: needinfo?(xidorn+moz)
The first chunk of layout/reftests/w3c-css/failures.list as well as layout/reftests/w3c-css/import-tests.py is for the renaming from selectors-4 to selectors4, and the rest of layout/reftests/w3c-css/failures.list is for fixing issues introduced from bug 1351548.

I can separate them if you find it easier to review, but I think I would eventually squash them altogether...
Flags: needinfo?(xidorn+moz)
Comment on attachment 8865314 [details]
Bug 1362255 - Import CSS tests from web-platform-tests.

https://reviewboard.mozilla.org/r/136954/#review140182

r=dbaron with the following

::: layout/reftests/w3c-css/failures.list:170
(Diff revision 2)
>  fuzzy(87,180) css-multicol-1/multicol-columns-004.xht
>  fuzzy(87,180) css-multicol-1/multicol-columns-005.xht
>  fuzzy(87,180) css-multicol-1/multicol-columns-006.xht
>  fuzzy(87,180) css-multicol-1/multicol-columns-007.xht
>  fuzzy(204,930) fuzzy-if(skiaContent,208,930) css-multicol-1/multicol-columns-invalid-001.xht
> -fails-if(!stylo)-if(OSX||winWidget) css-multicol-1/multicol-columns-invalid-002.xht
> +fails-if(OSX||winWidget) css-multicol-1/multicol-columns-invalid-002.xht

I'm surprised that the "old" line here isn't a syntax error for the reftest manifest parser.  Could you file a bug on the reftest harness about that?  I'm guessing the problem is that perhaps we aren't properly checking the result of Components.utils.evalInSandbox when evaluating expressions like "!stylo)-if(OSX||winWidget"

::: layout/reftests/w3c-css/import-tests.py
(Diff revision 2)
> -                  stdout=PIPE, cwd=gSrcPath))
> -    gLog.write("\nfrom repository: ")
> -    log_output_of(Popen(["hg", "paths", "default"],

Maybe use "git remote get-url origin" instead of dropping this?
Attachment #8865314 - Flags: review?(dbaron) → review+
Comment on attachment 8865314 [details]
Bug 1362255 - Import CSS tests from web-platform-tests.

https://reviewboard.mozilla.org/r/136954/#review140182

> Maybe use "git remote get-url origin" instead of dropping this?

Hmmm... but locally I use origin for my own fork and upstream for the official wpt repo...

I failed to find an easy way to get the url of the current branch...
Comment on attachment 8865314 [details]
Bug 1362255 - Import CSS tests from web-platform-tests.

https://reviewboard.mozilla.org/r/136954/#review140182

> I'm surprised that the "old" line here isn't a syntax error for the reftest manifest parser.  Could you file a bug on the reftest harness about that?  I'm guessing the problem is that perhaps we aren't properly checking the result of Components.utils.evalInSandbox when evaluating expressions like "!stylo)-if(OSX||winWidget"

The old lines here are syntax error for reftest manifest parser, but they were not propagated to reftest.list because the person who did this change didn't rerun the import script.
Comment on attachment 8865314 [details]
Bug 1362255 - Import CSS tests from web-platform-tests.

https://reviewboard.mozilla.org/r/136954/#review140182

> Hmmm... but locally I use origin for my own fork and upstream for the official wpt repo...
> 
> I failed to find an easy way to get the url of the current branch...

Add a bit more complicated thing to get the upstream of current branch, then use `git remote get-url` with the upstream name. I think that should work...
Going to land the import script without importing the new tests... We need to mark failures of the new tests correctly in failures.list which could be time consuming, and I don't quite have much time for now...

This is the last try run I did:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cbb7f151ce924c0959ea3a2b09afc104daf241a
Keywords: leave-open
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9321763da68
Import CSS tests from web-platform-tests. r=dbaron
For who may want to help here, you need to:
1. checkout a web-platform-tests clone from https://github.com/w3c/web-platform-tests.git
2. run the script via "python layout/reftests/w3c-css/import-tests.py <path-to-wpt-clone>"
3. "hg add layout/reftests/w3c-css/received" and commit
4. push to try with reftests run (probably start with only linux64 platform)
5. update layout/reftests/w3c-css/failures.list with new failures appear in your try run
6. rerun the script to update reftest.list
7. commit and push to try with reftests run on all platforms and ensure they are all green, otherwise go back to step 5
8. land the new tests

It would be good to also file bugs for any new failures, but that may take more time... I guess we really want to land the tests first, and we can triage them later.
Reassign to Louis to follow up the actions in comment 15.
Assignee: xidorn+moz → lochang
Status: NEW → ASSIGNED
Priority: -- → P3
Comment on attachment 8872943 [details]
Bug 1362255 Part 1 - Update failures.list for new failures.

Hi Xidron,

I have added the failure tests into failure lists. I will follow up these failures later. Could you take a look? Thanks.

Try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=887a2e7fc135e7aa676e9988d15cc3cfb94059ad
Attachment #8872943 - Attachment description: Bug 1362255 - Update failures.list for new failures. f?xidorn → Bug 1362255 - Update failures.list for new failures.
Attachment #8872943 - Flags: feedback?(xidorn+moz)
Comment on attachment 8872943 [details]
Bug 1362255 Part 1 - Update failures.list for new failures.

So you are going to just disable every test which fails anywhere... which is probably fine for now if you can eventually triage and mark them properly.

But it seems you are disabling many existing tests. Why do you need to do that? I suppose most of that is probably that those tests are failing in stylo, or pass in stylo but not in non-stylo? Those should really be marked with fails-if(stylo||styloVsGecko) or fails-if(!stylo) accordingly rather than disabled directly.

People may have changed the annotation directly in reftest.list... That makes me wonder that we probably should make the script smarter and try to keep the annotations rather than using a separate failures.list... But that sounds non-trivial, and I don't know whether it's worth...

For now, could you look at the difference of reftest.list, and try not to disable any existing test via copying some annotation there back to failures.list accordingly?
Attachment #8872943 - Flags: feedback?(xidorn+moz) → feedback-
failing tests should be annotated as "fails", not "skip".  Then we get an UNEXPECTED-PASS if we fix the failure, which means we'll fix the annotation when we fix the failure, which means we'll then get coverage against regressions of that fix.
Comment on attachment 8872943 [details]
Bug 1362255 Part 1 - Update failures.list for new failures.

Hi Xidorn,

Could you please review the patch again for me? Thanks.
I have fixed the patch based on your comment.
The modifications includes:
1) skip focus-within-shadow-*.html tests due to test method changed and causing test timeout
2) sync the failures.list with reftest.list
3) add new failures to the list

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f3f54e7cbed104514376b43ef291b371766bc74
Attachment #8872943 - Flags: feedback?(xidorn+moz)
Comment on attachment 8872943 [details]
Bug 1362255 Part 1 - Update failures.list for new failures.

https://reviewboard.mozilla.org/r/144482/#review150534

::: layout/reftests/w3c-css/failures.list:274
(Diff revision 2)
> +fuzzy(50,160) css-values-3/ch-unit-002.html
> +fails-if(!styloVsGecko) css-values-3/ch-unit-003.html
> +fuzzy(78,197) css-values-3/ch-unit-004.html
> +fails-if(!styloVsGecko) css-writing-modes-3/float-lft-orthog-htb-in-vlr-002.xht
> +fails-if(!styloVsGecko) css-writing-modes-3/float-lft-orthog-htb-in-vrl-002.xht
> +fails-if(!styloVsGecko) css-writing-modes-3/float-lft-orthog-vlr-in-htb-002.xht
> +fails-if(!styloVsGecko) css-writing-modes-3/float-lft-orthog-vrl-in-htb-002.xht
> +fails-if(!styloVsGecko) css-writing-modes-3/float-rgt-orthog-htb-in-vlr-003.xht
> +fails-if(!styloVsGecko) css-writing-modes-3/float-rgt-orthog-htb-in-vrl-003.xht
> +fails-if(!styloVsGecko) css-writing-modes-3/float-rgt-orthog-vlr-in-htb-003.xht
> +fails-if(!styloVsGecko) css-writing-modes-3/float-rgt-orthog-vrl-in-htb-003.xht
> +fails-if(!styloVsGecko) css-writing-modes-3/sizing-orthog-htb-in-vrl-001.xht
> +fails-if(!styloVsGecko) css-writing-modes-3/sizing-orthog-htb-in-vrl-004.xht
> +fails-if(!styloVsGecko) css-writing-modes-3/sizing-orthog-htb-in-vrl-013.xht
> +fuzzy(255,1455) css-writing-modes-3/sizing-orthog-htb-in-vlr-008.xht
> +fuzzy(255,1455) css-writing-modes-3/sizing-orthog-htb-in-vlr-020.xht
> +fuzzy(255,5675) css-writing-modes-3/sizing-orthog-htb-in-vrl-008.xht
> +fuzzy(255,5675) css-writing-modes-3/sizing-orthog-htb-in-vrl-020.xht
> +fuzzy(255,1264) css-writing-modes-3/sizing-orthog-vlr-in-htb-008.xht
> +fuzzy(255,1264) css-writing-modes-3/sizing-orthog-vlr-in-htb-020.xht
> +fuzzy(255,1264) css-writing-modes-3/sizing-orthog-vrl-in-htb-008.xht
> +fuzzy(255,1264) css-writing-modes-3/sizing-orthog-vrl-in-htb-020.xht
> +fuzzy-if(Android,255,1225) css-writing-modes-3/sizing-orthog-htb-in-vlr-003.xht
> +fuzzy-if(Android,255,1225) css-writing-modes-3/sizing-orthog-htb-in-vlr-009.xht
> +fuzzy-if(Android,255,1225) css-writing-modes-3/sizing-orthog-htb-in-vlr-015.xht
> +fuzzy-if(Android,255,1225) css-writing-modes-3/sizing-orthog-htb-in-vlr-021.xht
> +fuzzy-if(Android,255,2482) css-writing-modes-3/sizing-orthog-htb-in-vrl-003.xht
> +fuzzy-if(Android,255,2482) css-writing-modes-3/sizing-orthog-htb-in-vrl-009.xht
> +fuzzy-if(Android,255,2482) css-writing-modes-3/sizing-orthog-htb-in-vrl-015.xht
> +fuzzy-if(Android,255,2482) css-writing-modes-3/sizing-orthog-htb-in-vrl-021.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vlr-in-htb-003.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vlr-in-htb-009.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vlr-in-htb-015.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vlr-in-htb-021.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vrl-in-htb-003.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vrl-in-htb-009.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vrl-in-htb-015.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vrl-in-htb-021.xht

Please move them to be next to tests of the same spec.

::: layout/reftests/w3c-css/failures.list:288
(Diff revision 2)
> +fuzzy(255,1455) css-writing-modes-3/sizing-orthog-htb-in-vlr-008.xht
> +fuzzy(255,1455) css-writing-modes-3/sizing-orthog-htb-in-vlr-020.xht
> +fuzzy(255,5675) css-writing-modes-3/sizing-orthog-htb-in-vrl-008.xht
> +fuzzy(255,5675) css-writing-modes-3/sizing-orthog-htb-in-vrl-020.xht
> +fuzzy(255,1264) css-writing-modes-3/sizing-orthog-vlr-in-htb-008.xht
> +fuzzy(255,1264) css-writing-modes-3/sizing-orthog-vlr-in-htb-020.xht
> +fuzzy(255,1264) css-writing-modes-3/sizing-orthog-vrl-in-htb-008.xht
> +fuzzy(255,1264) css-writing-modes-3/sizing-orthog-vrl-in-htb-020.xht
> +fuzzy-if(Android,255,1225) css-writing-modes-3/sizing-orthog-htb-in-vlr-003.xht
> +fuzzy-if(Android,255,1225) css-writing-modes-3/sizing-orthog-htb-in-vlr-009.xht
> +fuzzy-if(Android,255,1225) css-writing-modes-3/sizing-orthog-htb-in-vlr-015.xht
> +fuzzy-if(Android,255,1225) css-writing-modes-3/sizing-orthog-htb-in-vlr-021.xht
> +fuzzy-if(Android,255,2482) css-writing-modes-3/sizing-orthog-htb-in-vrl-003.xht
> +fuzzy-if(Android,255,2482) css-writing-modes-3/sizing-orthog-htb-in-vrl-009.xht
> +fuzzy-if(Android,255,2482) css-writing-modes-3/sizing-orthog-htb-in-vrl-015.xht
> +fuzzy-if(Android,255,2482) css-writing-modes-3/sizing-orthog-htb-in-vrl-021.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vlr-in-htb-003.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vlr-in-htb-009.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vlr-in-htb-015.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vlr-in-htb-021.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vrl-in-htb-003.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vrl-in-htb-009.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vrl-in-htb-015.xht
> +fuzzy-if(Android,255,1074) css-writing-modes-3/sizing-orthog-vrl-in-htb-021.xht

These fuzzy numbers look pretty big. Have you used Reftest Analyser to confirm that they are really fuzzy, rather than failures?
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #25)
> Comment on attachment 8872943 [details]
> Bug 1362255 - Update failures.list for new failures.
> 
> These fuzzy numbers look pretty big. Have you used Reftest Analyser to
> confirm that they are really fuzzy, rather than failures?

I have looked into reftest analyzer of these test cases:
fuzzy(255,1455) css-writing-modes-3/sizing-orthog-htb-in-vlr-008.xht
fuzzy(255,1455) css-writing-modes-3/sizing-orthog-htb-in-vlr-020.xht
fuzzy(255,5675) css-writing-modes-3/sizing-orthog-htb-in-vrl-008.xht
fuzzy(255,5675) css-writing-modes-3/sizing-orthog-htb-in-vrl-020.xht
fuzzy(255,1264) css-writing-modes-3/sizing-orthog-vlr-in-htb-008.xht
fuzzy(255,1264) css-writing-modes-3/sizing-orthog-vlr-in-htb-020.xht
fuzzy(255,1264) css-writing-modes-3/sizing-orthog-vrl-in-htb-008.xht
fuzzy(255,1264) css-writing-modes-3/sizing-orthog-vrl-in-htb-020.xht
and they seem fuzzy to me.

But I couldn't open reftest analzyer for Android, it keeps loading log...
I will try to download log_backing.log and open it locally.

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f1a008d290f1c66cac8f1acebe811106ff2bd82&selectedJob=104849806
(In reply to Louis Chang [:louis] from comment #26)
> But I couldn't open reftest analzyer for Android, it keeps loading log...
> I will try to download log_backing.log and open it locally.

Filed bug 1370840 for this.
So, the tests you marked fuzzy are actually wrong test. Those tests rely on that 50 digits take 50ch width. However, those tests use neither monospace font, nor all zero, which means nothing guarantees the text would take 50ch, so they are just broken.

Gérard, it seems you are the author of those tests, could you fix them via either using all zero or using monospace font?
Flags: needinfo?(bugzilla)
Hello Xidorn and Louis,

Thank you both for your comment, analysis and feedback: they are useful and constructive.

Right now, I am enclined to prefer to fix those 8 tests and their associated reference files by specifying the use of monospace font. (By the way, I wonder/believe I should probably do the same to all the other sizing-orthog-* tests and associated reference files.)

There is at least 1 problem that will delay the fixing of those 8 tests and their associated reference files by me. I am not familiar with how to use GitHub right now. I do not understand how to use GitHub to submit/modify/remove/whatever web-platform-tests tests. And this is going to take me time... (eg I can not even reach, access or can not even see any tests after the first 1000 file records in 
https://github.com/w3c/web-platform-tests/tree/master/css/css-writing-modes-3
)

Also, I am seriously considering switching (alone! with no outside help) my operating system to Debian 9 (codename 'stretch') which is supposed to be released on June 17th 2017, which is in 10 days from now. 

So, would it be okay if I could fix those tests after, say, July 1st? 

Otherwise, if you can not wait and you, Xidorn, want to fix those 8 tests and their associated reference files by specifying the use of monospace font, then you have my consent and approval to go ahead and do it:


          body
            {
              font-size: 16px;
insertion:    font-family: monospace;
              line-height: 1.25; /* therefore, each line box is 20px tall */
Flags: needinfo?(bugzilla)
Hi Xidorn,

Could you please review the patch again? Thanks.
I have change the sizing-orthog-* tests to fails and move all the tests next to the same spec.
Comment on attachment 8872943 [details]
Bug 1362255 Part 1 - Update failures.list for new failures.

https://reviewboard.mozilla.org/r/144482/#review151090
Attachment #8872943 - Flags: review?(xidorn+moz) → review+
(In reply to Louis Chang [:louis] from comment #34)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=89206baf4c22562c9b76e599667fcebfac2c3a14

There are some new fuzzy fails, I will fix them before landing the patch.
Attachment #8865315 - Attachment is obsolete: true
Hi Xidorn,

I have updated the failures.list for the new fails due to bug 1369264 and bug 1347410 landed recently (Part I).

And could you please review the part 2 patch which generates the reftest.list based on the new failures.list also import the new wpt tests?
Comment on attachment 8875965 [details]
Bug 1362255 Part 2 - Import the tests.

https://reviewboard.mozilla.org/r/147372/#review151630

Looks good to me. Thanks for working on this.
Attachment #8875965 - Flags: review?(xidorn+moz) → review+
Keywords: checkin-needed
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/87c849a40f9d
Part 1 - Update failures.list for new failures. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/7c9d96bbc400
Part 2 - Import the tests. r=xidorn
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/87c849a40f9d
https://hg.mozilla.org/mozilla-central/rev/7c9d96bbc400
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
> I made https://github.com/w3c/web-platform-tests/pull/6196 for the tests
> mentioned in comment 26, comment 28, and comment 29.

Great! Thank you, David.

One question though, if I may... I am very much confused. 

You created this commit:

https://github.com/w3c/web-platform-tests/pull/6196/commits/9b0411e5883a4c6bad2ca91b139754b543de9075

and according to this file

https://github.com/w3c/web-platform-tests/pull/6196

upsuper (Xidorn) approved it ("approved these changes") 15 hours ago. So why does this webpage

https://github.com/w3c/web-platform-tests/pulls

say that a review is (still) requested?
https://github.com/w3c/web-platform-tests/pull/6196 says, in the box just above the comment field at the bottom:

> Review required
> At least one approved review is required by reviewers with write access. 

xidorn doesn't have write access to the repository, so his review doesn't count for mergability.
> https://github.com/w3c/web-platform-tests/pull/6196 says, in the box just
> above the comment field at the bottom:
> 
> > Review required
> > At least one approved review is required by reviewers with write access. 

I do not see this; I even searched (Ctrl+F) the source code of that webpage. Presumably, the text and/or tooltips are different when you are signed in as a participant or contributor or reviewer. Already here, I would not be able to state or explain the difference between a participant and a contributor (assuming that a reviewer is defined as someone with write access privileges).

> xidorn doesn't have write access to the repository, so his review doesn't
> count for mergability.

I read
"Only reviews by reviewers with write access count toward mergeability"
from
<div class="discussion-item-icon tooltipped tooltipped-ne" aria-label="Only reviews by reviewers with write access count toward mergeability">
when I hover the cursor over the gray checkmark... which is definitely not an intuitive (it's counter-intuitive IMO) image for such kind of notification and also tooltip text is not an ideal way to communicate detailed information. 
Furthermore, in the upper-right side column of the pull/6196 webpage, labeled "Reviewers", I can read, see upsuper in the list (again with a gray checkmark) of reviewers.

Okay. Thank you for your reply, David. I will try to move on from there...
Blocks: 1380924
You need to log in before you can comment on or make changes to this bug.