[css-transforms] Add one extra expected value for the float limit test on transform #19557
Conversation
|
@stephenmcgruer, could you please review this? Thanks. |
|
@emilio would you mind reviewing this PR? |
|
Or I could drop these limit tests. (Note: Webkit dumps something like |
|
Apologies for the delay on reviewing (no excuses, just bad time management). I will look at this in the next 24 hours.
These limit tests have come up before. We have a general issue nowadays that Chromium is pushing hard to write most tests in WPT, but sometimes what we think is reasonable turns out not to be for other browsers (no idea yet if that's true for this case). That's not bad necessarily, but on multiple occasions I've come across upstream WPT PRs that have removed tests that Chromium added, without any Chromium involvement (thank you for adding me to this one! :)), and so we lose code coverage without knowing about it which is scary :(. There's no actual action item from the above, it's just concerning me atm. @foolip as FYI |
…t on transform. Based on the [spec](https://drafts.csswg.org/css-values-4/#numbers), as with integers, the first character of a number may be immediately preceded by - or + to indicate the number’s sign. So it should be ok to not add the "+" before the integer. Gecko doesn't serialize the "+", but Blink serializes it, so we should accept two different valid results. Besides, WebKit uses 2e+80 as the limit. (Note: perhaps we shouldn't add limit test here.)
BorisChiou
force-pushed
the
transform/parsing
branch
from
a2e670f
to
1b64586
Compare
|
I see. I keep the tests for now, but it'd be great if you can move these tests into Chromium repo in the future. :) |
|
Ok, so https://drafts.csswg.org/css-values-4/#numeric-ranges confirms that its UA-dependent what the limits should be. Therefore this test should not have been WPT. I'm going to fire up a Chromium CL to move this test (and any others I can find) back into Chromium. Sorry for the trouble. |
This test tests UA-specific behavior; see #19557 Bug: None Change-Id: I650dfa3c59488ad44c4b91f70be5144fd56f6024
|
They will be removed in #19646; I wasn't able to find any other limit tests that actually got exported to WPT, but please keep an eye out and feel free to fire issues my way if you do. They're easy to port back into our internal repo :) |
|
Thanks @stephenmcgruer for moving the tests in Chromium. I have yet to see something very bad happen as a result of Chromium-originating tests being changed or removed in WPT, but the possibility is enough to make me a bit nervous. What would tooling look like that allowed us to notice when this is about to happen, or has already happened? |
This test tests UA-specific behavior; see web-platform-tests/wpt#19557 Bug: None Change-Id: I650dfa3c59488ad44c4b91f70be5144fd56f6024 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1856860 Reviewed-by: Xida Chen <xidachen@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#705296}
This test tests UA-specific behavior; see #19557 Bug: None Change-Id: I650dfa3c59488ad44c4b91f70be5144fd56f6024 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1856860 Reviewed-by: Xida Chen <xidachen@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#705296}
This test tests UA-specific behavior; see #19557 Bug: None Change-Id: I650dfa3c59488ad44c4b91f70be5144fd56f6024 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1856860 Reviewed-by: Xida Chen <xidachen@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#705296}
|
Thanks. Close this PR. |
I don't know actually. Maybe try to search |
|
I think Philip is talking slightly wider than just a limit test :). There is a general issue that once browsers start to move tests to WPT (and have an automated sync), they allow external contributors to have the power to remove tests. @foolip - my guess is this would be best detected during import from WPT --> [browser], though I'm struggling to find an obvious heuristic. We could diff the tests ran before/after the import, and generate a list of (sub)tests that have been removed... that requires manual review so might not be great. Perhaps only doing it for tests that went pass --> removed in [browser], but that still doesn't seem like a useful heuristic... |
|
Vetting all removals does seem a bit excessive. @Hexles has written a script to figure out where a test "came from" so maybe we could use that and check removals in tests that were added in Chromium first? I expect that almost all will be OK, but maybe we can then identify what makes a removal problematic. |
…ernal, a=testonly Automatic update from web-platform-tests Move float-cast overflow test to wpt_internal This test tests UA-specific behavior; see web-platform-tests/wpt#19557 Bug: None Change-Id: I650dfa3c59488ad44c4b91f70be5144fd56f6024 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1856860 Reviewed-by: Xida Chen <xidachen@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#705296} -- wpt-commits: b87b15057aa1fb5d19892c0afc6aec5a801d9b03 wpt-pr: 19646
…ernal, a=testonly Automatic update from web-platform-tests Move float-cast overflow test to wpt_internal This test tests UA-specific behavior; see web-platform-tests/wpt#19557 Bug: None Change-Id: I650dfa3c59488ad44c4b91f70be5144fd56f6024 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1856860 Reviewed-by: Xida Chen <xidachen@chromium.org> Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#705296} -- wpt-commits: b87b15057aa1fb5d19892c0afc6aec5a801d9b03 wpt-pr: 19646
…ernal, a=testonly Automatic update from web-platform-tests Move float-cast overflow test to wpt_internal This test tests UA-specific behavior; see web-platform-tests/wpt#19557 Bug: None Change-Id: I650dfa3c59488ad44c4b91f70be5144fd56f6024 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1856860 Reviewed-by: Xida Chen <xidachenchromium.org> Commit-Queue: Stephen McGruer <smcgruerchromium.org> Cr-Commit-Position: refs/heads/master{#705296} -- wpt-commits: b87b15057aa1fb5d19892c0afc6aec5a801d9b03 wpt-pr: 19646 UltraBlame original commit: 9ff77afb7e61c717a73751eb8e7da2247f377e85
…ernal, a=testonly Automatic update from web-platform-tests Move float-cast overflow test to wpt_internal This test tests UA-specific behavior; see web-platform-tests/wpt#19557 Bug: None Change-Id: I650dfa3c59488ad44c4b91f70be5144fd56f6024 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1856860 Reviewed-by: Xida Chen <xidachenchromium.org> Commit-Queue: Stephen McGruer <smcgruerchromium.org> Cr-Commit-Position: refs/heads/master{#705296} -- wpt-commits: b87b15057aa1fb5d19892c0afc6aec5a801d9b03 wpt-pr: 19646 UltraBlame original commit: 9ff77afb7e61c717a73751eb8e7da2247f377e85
…ernal, a=testonly Automatic update from web-platform-tests Move float-cast overflow test to wpt_internal This test tests UA-specific behavior; see web-platform-tests/wpt#19557 Bug: None Change-Id: I650dfa3c59488ad44c4b91f70be5144fd56f6024 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1856860 Reviewed-by: Xida Chen <xidachenchromium.org> Commit-Queue: Stephen McGruer <smcgruerchromium.org> Cr-Commit-Position: refs/heads/master{#705296} -- wpt-commits: b87b15057aa1fb5d19892c0afc6aec5a801d9b03 wpt-pr: 19646 UltraBlame original commit: 9ff77afb7e61c717a73751eb8e7da2247f377e85
Based on the spec, as with integers, the first character of a number may be immediately preceded by - or + to indicate the number’s sign.
So it should be ok to not add the "+" before the integer. Gecko doesn't serialize the "+", but Blink serializes it, so we should accept two different valid results. (Note: perhaps we shouldn't add limit test here.)