Remove "interact" flags for tests that can be run automatically#5370
Conversation
|
Notifying @astearns, @atanassov, @bert-github, @chenxix, @dbaron, @fantasai, @frivoal, @hshiozawa, @kojiishi, @kwkbtr, @myakura, @plinss, @r12a, @snsk, @svgeesus, @tabatkins, and @upsuper. (Learn how reviewing works.) |
Chrome (unstable channel)Testing web-platform-tests at revision 4e42c4c All results29 tests ran/css/CSS2/backgrounds/background-root-012b.xht
/css/CSS2/backgrounds/background-root-013b.xht
/css/CSS2/backgrounds/background-root-014b.xht
/css/CSS2/backgrounds/background-root-017.xht
/css/css-regions-1/contentEditable/contentEditable-001.html
/css/css-regions-1/contentEditable/contentEditable-002.html
/css/css-regions-1/contentEditable/contentEditable-003.html
/css/css-regions-1/contentEditable/contentEditable-004.html
/css/css-regions-1/contentEditable/contentEditable-005.html
/css/css-regions-1/contentEditable/contentEditable-006.html
/css/css-regions-1/contentEditable/contentEditable-007.html
/css/css-regions-1/contentEditable/contentEditable-008.html
/css/css-regions-1/contentEditable/contentEditable-009.html
/css/css-regions-1/contentEditable/contentEditable-010.html
/css/css-regions-1/contentEditable/contentEditable-011.html
/css/css-writing-modes-3/background-position-vrl-018.xht
/css/css-writing-modes-3/background-position-vrl-020.xht
/css/css-writing-modes-3/background-position-vrl-022.xht
/css/selectors4/focus-within-001.html
/css/selectors4/focus-within-002.html
/css/selectors4/focus-within-003.html
/css/selectors4/focus-within-004.html
/css/selectors4/focus-within-005.html
/css/selectors4/focus-within-006.html
/css/selectors4/focus-within-shadow-001.html
/css/selectors4/focus-within-shadow-002.html
/css/selectors4/focus-within-shadow-003.html
/css/selectors4/focus-within-shadow-004.html
/css/selectors4/focus-within-shadow-005.html
|
frivoal
left a comment
frivoal
left a comment
There was a problem hiding this comment.
I've reviewed this because some tests for focus within where in there, and I've worked on that feature and some of these tests. I've reviewed the rest of the PR while I was at it, but feedback from someone familiar with the tests / specs being tests would probably be good.
Please see the individual comments from adjustments that should be made before merging.
| <link rel="author" title="Florian Rivoal" href="mailto:florian@rivoal.net"> | ||
| <link rel="help" href="https://drafts.csswg.org/selectors-4/#focus-within-pseudo"> | ||
| <link rel="match" href="focus-within-001-ref.html"> | ||
| <meta name="flags" content="interact"> |
There was a problem hiding this comment.
-
It is better to switch to
<meta name="flags" content="">than to remove the line. Makes it clear that you have considered the question and decided that there was no need for flags, instead of maybe forgetting to think about it. -
If you remove the interact flag, the comment above the javascript about it being an optional convenience becomes wrong and needs to be removed: either the test is interactive and the js is a optional helper, or it is automated and js is required.
-
As we switch to js required, it would be good to add
class="reftest-wait"on the root, and remove it once js has run, so that we make sure that the ref comparison is done at the right time, and that if js fails to run for some reason, we detect it as a test that failed to run rather than a failure to implement focus-within correctly.
There was a problem hiding this comment.
Ok, I've done this in all the tests.
I'm still removing the empty flags, but I can add it if it's a blocker to land this. I have seen many tests that don't have it.
| <link rel="author" title="Florian Rivoal" href="mailto:florian@rivoal.net"> | ||
| <link rel="help" href="https://drafts.csswg.org/selectors-4/#focus-within-pseudo"> | ||
| <link rel="match" href="focus-within-001-ref.html"> | ||
| <meta name="flags" content="interact"> |
There was a problem hiding this comment.
same comment as for focus-within-001
| <link rel="author" title="Florian Rivoal" href="mailto:florian@rivoal.net"> | ||
| <link rel="help" href="https://drafts.csswg.org/selectors-4/#focus-within-pseudo"> | ||
| <link rel="match" href="focus-within-001-ref.html"> | ||
| <meta name="flags" content="interact"> |
There was a problem hiding this comment.
same comment as for focus-within-001
| <link rel="author" title="Florian Rivoal" href="mailto:florian@rivoal.net"> | ||
| <link rel="help" href="https://drafts.csswg.org/selectors-4/#focus-within-pseudo"> | ||
| <link rel="match" href="focus-within-001-ref.html"> | ||
| <meta name="flags" content="interact"> |
There was a problem hiding this comment.
same comment as for focus-within-001
| <link rel="author" title="Florian Rivoal" href="mailto:florian@rivoal.net"> | ||
| <link rel="help" href="https://drafts.csswg.org/selectors-4/#focus-within-pseudo"> | ||
| <link rel="match" href="focus-within-001-ref.html"> | ||
| <meta name="flags" content="interact"> |
There was a problem hiding this comment.
same comment as for focus-within-001
| <link rel="help" href="http://www.w3.org/TR/css3-regions/#the-flow-into-property"> | ||
| <link rel="help" href="http://www.w3.org/TR/css3-regions/#flow-from"> | ||
| <link rel="help" href="http://www.w3.org/TR/html5/editing.html#contenteditable"> | ||
| <meta name="flags" content="dom interact"> |
There was a problem hiding this comment.
same comment as css-regions-1/contentEditable/contentEditable-001.html
| <link rel="help" href="http://www.w3.org/TR/css3-regions/#the-flow-into-property"> | ||
| <link rel="help" href="http://www.w3.org/TR/css3-regions/#flow-from"> | ||
| <link rel="help" href="http://www.w3.org/TR/html5/editing.html#contenteditable"> | ||
| <meta name="flags" content="dom interact"> |
There was a problem hiding this comment.
same comment as css-regions-1/contentEditable/contentEditable-001.html
| <link rel="help" href="http://www.w3.org/TR/css3-regions/#the-flow-into-property"> | ||
| <link rel="help" href="http://www.w3.org/TR/css3-regions/#flow-from"> | ||
| <link rel="help" href="http://www.w3.org/TR/html5/editing.html#contenteditable"> | ||
| <meta name="flags" content="dom interact"> |
There was a problem hiding this comment.
same comment as css-regions-1/contentEditable/contentEditable-001.html
| <link rel="help" href="http://www.w3.org/TR/css3-regions/#the-flow-into-property"> | ||
| <link rel="help" href="http://www.w3.org/TR/css3-regions/#flow-from"> | ||
| <link rel="help" href="http://www.w3.org/TR/html5/editing.html#contenteditable"> | ||
| <meta name="flags" content="dom interact"> |
There was a problem hiding this comment.
same comment as css-regions-1/contentEditable/contentEditable-001.html
| <link rel="help" href="http://www.w3.org/TR/css3-regions/#the-flow-into-property"> | ||
| <link rel="help" href="http://www.w3.org/TR/css3-regions/#flow-from"> | ||
| <link rel="help" href="http://www.w3.org/TR/html5/editing.html#contenteditable"> | ||
| <meta name="flags" content="dom interact"> |
There was a problem hiding this comment.
same comment as css-regions-1/contentEditable/contentEditable-001.html
|
Thanks @frivoal for the thorough review. I'll do the changes, but I'm not 100% sure about the empty <meta name="flags" content="">I cannot find any documentation saying that this is mandatory, and the initial examples in the doc don't use it. |
|
These tests are now available on w3c-test.org |
|
@frivoal I've applied the suggested changes, expect keeping the empty |
|
Per the readme, we shouldn't be editing things in |
|
@mrego I don't think there's any real preference either way. I'd rather not have them bloating the file, on the whole. |
|
@mrego: based on @gsnedders's comment, can you back out the changes to the tests under Once that's done, I'll merge. |
|
Ok, I've just reverted the changes on the Mozilla tests, I'll send them upstream to Firefox bugzilla. |
|
@mrego actually, I see that you've already merged master in. @gsnedders, any idea what travis is still complaining about? |
|
Probably jugglinmike/chrome-screenshot-race#1. |
|
I'm not sure if I'm checking the right thing, but the results I see don't make a lot of sense to me. The feature is not implemented yet, so it's impossible that the test is passing. |
mrego
force-pushed
the
remove-interact-flag
branch
from
bf87a39 to
e305c97
Compare
|
@gsnedders @jugglinmike @mrego
Right, the test results don't make sense to me either. Here and in other ref-tests PR I've been working on (#5285, #5283). If the Chrome stability test isn't working right due to jugglinmike/chrome-screenshot-race#1 or whatever, shouldn't we disable it until it is fixed? For now it's doing more harm than good. |
|
The latest Travis build seems to have run into #5407 |
|
JFTR, Mozilla tests have been modified upstream: https://bugzilla.mozilla.org/show_bug.cgi?id=1354006 |
There were a bunch of tests that have both a reference file and the "interact" flag. They use some small JavaScript to execute some steps before checking the output against reference file. This caused them to be wrongly considered manual tests when they can be run automatically. For more info check w3c/wpt-tools#196 and w3c/wpt-tools#197.
…flag Basically this patch does the following: * Duplicate some tests so now they have an automatic part and a manual one (as it's required to resize the window). * Use "reftest-wait" class to be sure that the JavaScript steps have been completed before checking the reference. * Remove some wrong "reftest-wait" used outside the root element. * Remove some wrong comments now that the tests are automatic.
mrego
force-pushed
the
remove-interact-flag
branch
from
e305c97 to
8f7d277
Compare
…tests. r=xidorn This patch has been extracted from: web-platform-tests/wpt#5370 The problem is that the "interact" flag on these tests causes that they are considered manual, when they can be run automatically. This patch removes the "interact" flag and uses "reftest-wait" class to be sure that the test has been completed.
|
Ok, it seems we can merge this now. Nice! |
…tests. r=xidorn This patch has been extracted from: web-platform-tests/wpt#5370 The problem is that the "interact" flag on these tests causes that they are considered manual, when they can be run automatically. This patch removes the "interact" flag and uses "reftest-wait" class to be sure that the test has been completed.
…tests. r=xidorn This patch has been extracted from: web-platform-tests/wpt#5370 The problem is that the "interact" flag on these tests causes that they are considered manual, when they can be run automatically. This patch removes the "interact" flag and uses "reftest-wait" class to be sure that the test has been completed.
…tests. r=xidorn This patch has been extracted from: web-platform-tests/wpt#5370 The problem is that the "interact" flag on these tests causes that they are considered manual, when they can be run automatically. This patch removes the "interact" flag and uses "reftest-wait" class to be sure that the test has been completed. UltraBlame original commit: 83eeccc86babcfc64c268fb7fad04af05470578c
…tests. r=xidorn This patch has been extracted from: web-platform-tests/wpt#5370 The problem is that the "interact" flag on these tests causes that they are considered manual, when they can be run automatically. This patch removes the "interact" flag and uses "reftest-wait" class to be sure that the test has been completed. UltraBlame original commit: 83eeccc86babcfc64c268fb7fad04af05470578c
…tests. r=xidorn This patch has been extracted from: web-platform-tests/wpt#5370 The problem is that the "interact" flag on these tests causes that they are considered manual, when they can be run automatically. This patch removes the "interact" flag and uses "reftest-wait" class to be sure that the test has been completed. UltraBlame original commit: 83eeccc86babcfc64c268fb7fad04af05470578c
…tests. r=xidorn This patch has been extracted from: web-platform-tests/wpt#5370 The problem is that the "interact" flag on these tests causes that they are considered manual, when they can be run automatically. This patch removes the "interact" flag and uses "reftest-wait" class to be sure that the test has been completed.
4 participants
There were a bunch of tests that have both a reference file and
the "interact" flag.
They use some small JavaScript to execute some steps
before checking the output against reference file.
This caused them to be wrongly considered manual tests
when they can be run automatically.
For more info check w3c/wpt-tools#196 and w3c/wpt-tools#197.
This change is