Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2833483002:
Enable wpt/css/css-ui-3 tests. (Closed)
|
Created:
3 years, 8 months ago by qyearsley Modified:
3 years, 8 months ago Reviewers:
Manuel Rego CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable wpt/css/css-ui-3 tests.
BUG=706118
Review-Url: https://codereview.chromium.org/2833483002
Cr-Commit-Position: refs/heads/master@{#466593}
Committed: https://chromium.googlesource.com/chromium/src/+/a2e0740bfc1942838a2b3273b10b65a6d0273019
Patch Set 1 #Patch Set 2 : Skip manual tests #
Total comments: 1
Patch Set 3 : Rebase and add bug associated with css-ui-3/caret-color-006.html #Patch Set 4 : rm editing/caret copies, move baselines #Patch Set 5 : Revert to previous patchset state #
Messages
Total messages: 22 (14 generated)
qyearsley@chromium.org changed reviewers: + rego@igalia.com
Note: I expect to have to rebaseline some tests before submitting this; we should check any new baselines to make sure they make sense.
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some comments about caret-color tests. Let's see what's the best solution we can find for this. https://codereview.chromium.org/2833483002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/NeverFixTests (right): https://codereview.chromium.org/2833483002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/NeverFixTests:267: external/wpt/css/css-ui-3/caret-color-017.html [ WontFix ] I'm not sure what's the best approach for these "caret-color" tests. Before they were not imported into "external/csswg-test/" because of "interact" flag. So I imported them manually under "editing/caret/" because, despite they don't have reference, they are useful as pixel tests (comparing against a PNG). Not all the tests are valid for this (as some have animations), but currently you can find the following ones under "editing/caret/". * caret-color-001.html * caret-color-002.html * caret-color-003.html * caret-color-004.html * caret-color-005.html * caret-color-007.html * caret-color-010.html * caret-color-011.html * caret-color-012.html * caret-color-014.html * caret-color-015.html * caret-color-016.html "caret-color-006.html" is not there because of it's an optional part of the spec that we haven't implemented (see http://crbug.com/713587 for more info). So I'd move "caret-color-006.html" "caret-color-008.html" test an animation and "caret-color-017.html" a transition. So it's fine we keep them in this file as "WontFix". Then we've 2 more special cases: "caret-color-015.html" and "caret-color-016.html". They have the following comment: <!-- This is a copy of the W3C test just changing the href on the link, so Blink considers it a visited link. --> So I guess these 2 should also stay in this file as "WontFix" and we should keep our custom copies under "editing/caret/". Summarizing my proposal would be: * Add "caret-color-006.html" to TestExpectations as "Skip" linking to crbug.com/713587. * Keep "caret-color-008.html", "caret-color-015.html", "caret-color-016.html" and "caret-color-017.html" in this file as WontFix. * Remove the rest of caret-color tests from this file and copy the expectations from "editing/caret/". * Remove the duplicated tests that are not needed from "editing/caret/": * caret-color-001.html * caret-color-002.html * caret-color-003.html * caret-color-004.html * caret-color-005.html * caret-color-007.html * caret-color-010.html * caret-color-011.html * caret-color-012.html * caret-color-014.html WDYT? Sorry for all the mess with this. :-)
On 2017/04/20 at 08:04:54, rego wrote: > Some comments about caret-color tests. > Let's see what's the best solution we can find for this. > > https://codereview.chromium.org/2833483002/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/NeverFixTests (right): > > https://codereview.chromium.org/2833483002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/NeverFixTests:267: external/wpt/css/css-ui-3/caret-color-017.html [ WontFix ] > I'm not sure what's the best approach for these "caret-color" tests. > > Before they were not imported into "external/csswg-test/" > because of "interact" flag. > So I imported them manually under "editing/caret/" because, > despite they don't have reference, they are useful > as pixel tests (comparing against a PNG). > > Not all the tests are valid for this (as some have animations), > but currently you can find the following ones under "editing/caret/". > * caret-color-001.html > * caret-color-002.html > * caret-color-003.html > * caret-color-004.html > * caret-color-005.html > * caret-color-007.html > * caret-color-010.html > * caret-color-011.html > * caret-color-012.html > * caret-color-014.html > * caret-color-015.html > * caret-color-016.html > > "caret-color-006.html" is not there because of it's an optional part of the spec > that we haven't implemented (see http://crbug.com/713587 for more info). > So I'd move "caret-color-006.html" > > "caret-color-008.html" test an animation and "caret-color-017.html" > a transition. So it's fine we keep them in this file as "WontFix". > > Then we've 2 more special cases: "caret-color-015.html" and "caret-color-016.html". > They have the following comment: > <!-- This is a copy of the W3C test just changing the href on the link, > so Blink considers it a visited link. --> > So I guess these 2 should also stay in this file as "WontFix" > and we should keep our custom copies under "editing/caret/". > > Summarizing my proposal would be: > * Add "caret-color-006.html" to TestExpectations as "Skip" linking to crbug.com/713587. > * Keep "caret-color-008.html", "caret-color-015.html", "caret-color-016.html" > and "caret-color-017.html" in this file as WontFix. > * Remove the rest of caret-color tests from this file and copy the expectations > from "editing/caret/". > * Remove the duplicated tests that are not needed from "editing/caret/": > * caret-color-001.html > * caret-color-002.html > * caret-color-003.html > * caret-color-004.html > * caret-color-005.html > * caret-color-007.html > * caret-color-010.html > * caret-color-011.html > * caret-color-012.html > * caret-color-014.html > > WDYT? > > Sorry for all the mess with this. :-) Alright! I've now tried to make those changes and restarted try jobs, let me see if you see something wrong :-)
Thanks for doing it! But I found 2 problems. First one is my fault as my analysis was not 100% right. Sorry :-( 1) "caret-color-015.html" and "caret-color-016.html" have been modified upstream, so now they've a JS that modifies the history so the links are visited. That's why the new imported "caret-color-016.html" is passing without issues, and we should now generate the PNGs for caret-color-015.html. We could get rid of the custom tests in "editing/caret/" for this. 2) But now I detected a problem, I cannot generate the PNGs for the tests under "external/wpt/css/css-ui-3/". For example, I've applied this CL locally, I've modified "caret-color-001.html" to paint the caret in red and the test passes (it doesn't check the PNGs). However if (without this CL) I do the same change for the current "caret-color-001.html" under "editing/caret/" the test fails. I guess it can be something related to the manifest, but I don't know what's the issue right now.
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/21 at 10:22:14, rego wrote: > Thanks for doing it! But I found 2 problems. > > First one is my fault as my analysis was not 100% right. Sorry :-( > > 1) "caret-color-015.html" and "caret-color-016.html" have been modified upstream, > so now they've a JS that modifies the history so the links are visited. > That's why the new imported "caret-color-016.html" is passing without issues, > and we should now generate the PNGs for caret-color-015.html. > We could get rid of the custom tests in "editing/caret/" for this. > > 2) But now I detected a problem, I cannot generate the PNGs for the tests > under "external/wpt/css/css-ui-3/". > For example, I've applied this CL locally, I've modified > "caret-color-001.html" to paint the caret in red > and the test passes (it doesn't check the PNGs). > However if (without this CL) I do the same change for the current > "caret-color-001.html" under "editing/caret/" the test fails. > > I guess it can be something related to the manifest, > but I don't know what's the issue right now. This is a good question, and I'm not sure right now why it's not generating PNG results :-/ For now, how does it sound to just enable the tests in this CL, and then follow up an dedup in a separate CL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/21 22:01:41, qyearsley (OOO until May 8) wrote: > On 2017/04/21 at 10:22:14, rego wrote: > > Thanks for doing it! But I found 2 problems. > > > > First one is my fault as my analysis was not 100% right. Sorry :-( > > > > 1) "caret-color-015.html" and "caret-color-016.html" have been modified > upstream, > > so now they've a JS that modifies the history so the links are visited. > > That's why the new imported "caret-color-016.html" is passing without issues, > > and we should now generate the PNGs for caret-color-015.html. > > We could get rid of the custom tests in "editing/caret/" for this. > > > > 2) But now I detected a problem, I cannot generate the PNGs for the tests > > under "external/wpt/css/css-ui-3/". > > For example, I've applied this CL locally, I've modified > > "caret-color-001.html" to paint the caret in red > > and the test passes (it doesn't check the PNGs). > > However if (without this CL) I do the same change for the current > > "caret-color-001.html" under "editing/caret/" the test fails. > > > > I guess it can be something related to the manifest, > > but I don't know what's the issue right now. > > This is a good question, and I'm not sure right now why it's not generating PNG > results :-/ Ok, so we've to investigate this at some point. > For now, how does it sound to just enable the tests in this CL, and then follow > up an dedup in a separate CL? It sounds good. I've reported http://crbug.com/714516 to track this. LGTM
The CQ bit was checked by rego@igalia.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493016982969000,
"parent_rev": "c6d7406702779c75f188e7380dd71f211ed4e765", "commit_rev":
"a2e0740bfc1942838a2b3273b10b65a6d0273019"}
Message was sent while issue was closed.
Description was changed from ========== Enable wpt/css/css-ui-3 tests. BUG=706118 ========== to ========== Enable wpt/css/css-ui-3 tests. BUG=706118 Review-Url: https://codereview.chromium.org/2833483002 Cr-Commit-Position: refs/heads/master@{#466593} Committed: https://chromium.googlesource.com/chromium/src/+/a2e0740bfc1942838a2b3273b10b... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/a2e0740bfc1942838a2b3273b10b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
