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

Status: RESOLVED FIXED

(Testing :: Reftest, enhancement, P3)

Description

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1362255 - Import CSS tests from web-platform-tests.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1362255 - Import the tests.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8865314 [details]
Bug 1362255 - Import CSS tests from web-platform-tests.

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

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8865315 [details]
Bug 1362255 - Import the tests.

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

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There's a bunch of stuff in this patch that looks like it's fixes for a different patch.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

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 9

[Xidorn Quan [:xidorn] UTC+10]

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 10

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8865314 [details]
Bug 1362255 - Import CSS tests from web-platform-tests.

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

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8865315 [details]
Bug 1362255 - Import the tests.

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

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9321763da68
Import CSS tests from web-platform-tests. r=dbaron

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 16

[Astley Chen (inactive)]

Reassign to Louis to follow up the actions in comment 15.

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/a9321763da68

Comment 18

[Louis Chang [:louis]]

Attachment: Bug 1362255 Part 1 - Update failures.list for new failures.

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

Comment 19

[Louis Chang [:louis]]

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

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

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?

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 22

[Louis Chang [:louis]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc7b16dc7de3dd2add836d5b8a5d53edbe7a6344

Comment 23

[Louis Chang [:louis]]

Comment on attachment 8872943 [details]
Bug 1362255 Part 1 - Update failures.list for new failures.

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

Comment 24

[Louis Chang [:louis]]

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

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

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?

Comment 26

[Louis Chang [:louis]]

(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

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

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?

Comment 29

[Gérard Talbot]

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 */

Comment 30

[Louis Chang [:louis]]

Comment on attachment 8872943 [details]
Bug 1362255 Part 1 - Update failures.list for new failures.

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

Comment 31

[Louis Chang [:louis]]

Comment on attachment 8872943 [details]
Bug 1362255 Part 1 - Update failures.list for new failures.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144482/diff/3-4/

Comment 32

[Louis Chang [:louis]]

Comment on attachment 8872943 [details]
Bug 1362255 Part 1 - Update failures.list for new failures.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144482/diff/4-5/

Comment 33

[Louis Chang [:louis]]

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 34

[Louis Chang [:louis]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89206baf4c22562c9b76e599667fcebfac2c3a14

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8872943 [details]
Bug 1362255 Part 1 - Update failures.list for new failures.

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

Comment 36

[Louis Chang [:louis]]

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

Comment 37

[Louis Chang [:louis]]

Attachment: Bug 1362255 Part 2 - Import the tests.

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

Comment 38

[Louis Chang [:louis]]

Comment on attachment 8872943 [details]
Bug 1362255 Part 1 - Update failures.list for new failures.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144482/diff/5-6/

Comment 39

[Louis Chang [:louis]]

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 40

[Louis Chang [:louis]]

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=24200d51c06cd1d3c83d3946305dcb9abef845e8

Comment 41

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
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

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I made https://github.com/w3c/web-platform-tests/pull/6196 for the tests mentioned in comment 26, comment 28, and comment 29.

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87c849a40f9d
https://hg.mozilla.org/mozilla-central/rev/7c9d96bbc400

Comment 45

[Gérard Talbot]

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

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 47

[Gérard Talbot]

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