-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document test change proposal principles #604
Comments
cc @nt1m |
Having watched test changes through out the year, I can say that most test changes that go through without review (from everyone) are usually uncontroversial:
If style containment is the case you're referring to, the spec had some examples that were plain wrong, and some of the related tests did end up changing: In general I would prefer to not enforce the test change proposal processes to every single test change. Recent discussions are just a relatively small subset of test changes that go unreviewed, and introducing unnecessary churn is counterproductive IMO. It is OK to file a test change proposal however to question recent changes though. |
We do have enough data in the CI to tell that a) a test is an Interop test and b) that results got worse in some browsers. Therefore a policy-we-could-enforce-in-code would be requiring additional review for any test change that regresses an test included in Interop in any participating browser engine. |
A subtest relying on unit arithmetic snuck into one of the WPTs covered by the "Custom Properties" focus area of Interop 2024. As unit arithmetic is not covered by Interop 2024, the default resolution to this problem is to remove the whole file from Interop [1]. However, in this case, it's relatively easy and harmless to move the offending test to a separate file. [1] web-platform-tests/interop#604 Change-Id: Ic444d07288c2c8bb54595c61813d0217b3cc1032 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6097816 Reviewed-by: Daniil Sakhapov <[email protected]> Commit-Queue: Anders Hartvoll Ruud <[email protected]> Cr-Commit-Position: refs/heads/main@{#1397239}
A subtest relying on unit arithmetic snuck into one of the WPTs covered by the "Custom Properties" focus area of Interop 2024. As unit arithmetic is not covered by Interop 2024, the default resolution to this problem is to remove the whole file from Interop [1]. However, in this case, it's relatively easy and harmless to move the offending test to a separate file. [1] web-platform-tests/interop#604 Change-Id: Ic444d07288c2c8bb54595c61813d0217b3cc1032 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6097816 Reviewed-by: Daniil Sakhapov <[email protected]> Commit-Queue: Anders Hartvoll Ruud <[email protected]> Cr-Commit-Position: refs/heads/main@{#1397239}
A subtest relying on unit arithmetic snuck into one of the WPTs covered by the "Custom Properties" focus area of Interop 2024. As unit arithmetic is not covered by Interop 2024, the default resolution to this problem is to remove the whole file from Interop [1]. However, in this case, it's relatively easy and harmless to move the offending test to a separate file. [1] web-platform-tests/interop#604 Change-Id: Ic444d07288c2c8bb54595c61813d0217b3cc1032 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6097816 Reviewed-by: Daniil Sakhapov <[email protected]> Commit-Queue: Anders Hartvoll Ruud <[email protected]> Cr-Commit-Position: refs/heads/main@{#1397239}
…ithmetic separately, a=testonly Automatic update from web-platform-tests [css-properties-values-api] Test unit arithmetic separately A subtest relying on unit arithmetic snuck into one of the WPTs covered by the "Custom Properties" focus area of Interop 2024. As unit arithmetic is not covered by Interop 2024, the default resolution to this problem is to remove the whole file from Interop [1]. However, in this case, it's relatively easy and harmless to move the offending test to a separate file. [1] web-platform-tests/interop#604 Change-Id: Ic444d07288c2c8bb54595c61813d0217b3cc1032 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6097816 Reviewed-by: Daniil Sakhapov <[email protected]> Commit-Queue: Anders Hartvoll Ruud <[email protected]> Cr-Commit-Position: refs/heads/main@{#1397239} -- wpt-commits: 09eacd6963a7623ac88d83573a7d5a6afe58f129 wpt-pr: 49730
…ithmetic separately, a=testonly Automatic update from web-platform-tests [css-properties-values-api] Test unit arithmetic separately A subtest relying on unit arithmetic snuck into one of the WPTs covered by the "Custom Properties" focus area of Interop 2024. As unit arithmetic is not covered by Interop 2024, the default resolution to this problem is to remove the whole file from Interop [1]. However, in this case, it's relatively easy and harmless to move the offending test to a separate file. [1] web-platform-tests/interop#604 Change-Id: Ic444d07288c2c8bb54595c61813d0217b3cc1032 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6097816 Reviewed-by: Daniil Sakhapov <[email protected]> Commit-Queue: Anders Hartvoll Ruud <[email protected]> Cr-Commit-Position: refs/heads/main@{#1397239} -- wpt-commits: 09eacd6963a7623ac88d83573a7d5a6afe58f129 wpt-pr: 49730
…ithmetic separately, a=testonly Automatic update from web-platform-tests [css-properties-values-api] Test unit arithmetic separately A subtest relying on unit arithmetic snuck into one of the WPTs covered by the "Custom Properties" focus area of Interop 2024. As unit arithmetic is not covered by Interop 2024, the default resolution to this problem is to remove the whole file from Interop [1]. However, in this case, it's relatively easy and harmless to move the offending test to a separate file. [1] web-platform-tests/interop#604 Change-Id: Ic444d07288c2c8bb54595c61813d0217b3cc1032 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6097816 Reviewed-by: Daniil Sakhapov <sakhapovchromium.org> Commit-Queue: Anders Hartvoll Ruud <andruudchromium.org> Cr-Commit-Position: refs/heads/main{#1397239} -- wpt-commits: 09eacd6963a7623ac88d83573a7d5a6afe58f129 wpt-pr: 49730 UltraBlame original commit: 1e99c47c806ed0d2784b3d771989166151147d26
…ithmetic separately, a=testonly Automatic update from web-platform-tests [css-properties-values-api] Test unit arithmetic separately A subtest relying on unit arithmetic snuck into one of the WPTs covered by the "Custom Properties" focus area of Interop 2024. As unit arithmetic is not covered by Interop 2024, the default resolution to this problem is to remove the whole file from Interop [1]. However, in this case, it's relatively easy and harmless to move the offending test to a separate file. [1] web-platform-tests/interop#604 Change-Id: Ic444d07288c2c8bb54595c61813d0217b3cc1032 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6097816 Reviewed-by: Daniil Sakhapov <sakhapovchromium.org> Commit-Queue: Anders Hartvoll Ruud <andruudchromium.org> Cr-Commit-Position: refs/heads/main{#1397239} -- wpt-commits: 09eacd6963a7623ac88d83573a7d5a6afe58f129 wpt-pr: 49730 UltraBlame original commit: 1e99c47c806ed0d2784b3d771989166151147d26
…ithmetic separately, a=testonly Automatic update from web-platform-tests [css-properties-values-api] Test unit arithmetic separately A subtest relying on unit arithmetic snuck into one of the WPTs covered by the "Custom Properties" focus area of Interop 2024. As unit arithmetic is not covered by Interop 2024, the default resolution to this problem is to remove the whole file from Interop [1]. However, in this case, it's relatively easy and harmless to move the offending test to a separate file. [1] web-platform-tests/interop#604 Change-Id: Ic444d07288c2c8bb54595c61813d0217b3cc1032 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6097816 Reviewed-by: Daniil Sakhapov <sakhapovchromium.org> Commit-Queue: Anders Hartvoll Ruud <andruudchromium.org> Cr-Commit-Position: refs/heads/main{#1397239} -- wpt-commits: 09eacd6963a7623ac88d83573a7d5a6afe58f129 wpt-pr: 49730 UltraBlame original commit: 1e99c47c806ed0d2784b3d771989166151147d26
…ithmetic separately, a=testonly Automatic update from web-platform-tests [css-properties-values-api] Test unit arithmetic separately A subtest relying on unit arithmetic snuck into one of the WPTs covered by the "Custom Properties" focus area of Interop 2024. As unit arithmetic is not covered by Interop 2024, the default resolution to this problem is to remove the whole file from Interop [1]. However, in this case, it's relatively easy and harmless to move the offending test to a separate file. [1] web-platform-tests/interop#604 Change-Id: Ic444d07288c2c8bb54595c61813d0217b3cc1032 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6097816 Reviewed-by: Daniil Sakhapov <[email protected]> Commit-Queue: Anders Hartvoll Ruud <[email protected]> Cr-Commit-Position: refs/heads/main@{#1397239} -- wpt-commits: 09eacd6963a7623ac88d83573a7d5a6afe58f129 wpt-pr: 49730
In #599 we had few discussions about test change proposals, and @chrishtr suggested that we document the underlying principles for how we make such decisions.
What we have been doing so far:
Test changes that are motivated by spec changes need special handling. It doesn't make sense to revert the change and keep tests that no longer match the spec. We haven't had this situation often, but I believe our default should be to drop the tests from interop if there is no better solution everyone agrees on.
We've also had one case of spec clarifications that didn't result in any tests changing, but the spec and tests now align. This is a point of discussion, but my personal view is that our default should be to keep the tests, as the tests are what we reviewed, and the failures are what we plan work around.
The text was updated successfully, but these errors were encountered: