Conversation
89c5260 to
4e2ad92
Compare
marcoscaceres
left a comment
There was a problem hiding this comment.
approving payment request test rename. Thanks!
FremyCompany
left a comment
There was a problem hiding this comment.
At first glance, the css-tables related renames seem ok
svgeesus
left a comment
There was a problem hiding this comment.
The renames to avoid -crash where it shouldn't be seem good. I skimmed over the python changes but someone else should review those.
gsnedders
left a comment
There was a problem hiding this comment.
Files that shouldn't have been renamed:
- adjoining-float-new-fc-thcrash.html (doesn't meet the criteria in the commit message)
- gradient-thcrash.html (is a reftest?!)
- extremely-tall-multicol-with-extremely-tall-child-thcrash.html (doesn't meet the criteria in the commit message)
- before-in-display-none-thcrash.html (ditto)
- shape-outside-inset-thcrash.html (is a reftest)
I'm not gonna bother going through the rest, something has gone wrong…
gsnedders
left a comment
There was a problem hiding this comment.
css/css-position/position-absolute-crash-chrome-005.html and css/css-scoping/whitespace-crash-001.html don't need renamed (they're not crash tests).
I also wonder if we should have a lint for -crash tests containing testharness.js? It really is an antipattern, but also something Chrome developers have already done (and AFAIK they run differently as a result in run-webkit-tests, @Hexcles?).
For testharness tests the following criteria were applied: * Test includes an assert * Test includes something other than synchronous tests For reftests all existing tests were renamed
Fixed. |
They could be rewritten to real tests though, but that's for another day.
I agree. We should.
Sorry I'm not sure what you're asking? Until today's introduction of crash tests, Chrome developers have written some de facto crash tests with dummy testharness asserts in them. These should not be needed in the future (at least not named with |
I think we could leave that to a followup? It doesn't seem critical to landing the patch, and there are already a bunch of exceptions which we could/should fix at the same time, but deciding how to proceed with that is more work. |
AIUI, does run-webkit-tests not run But to me I'm happy to leave it as a followup. |
That's new to me, but it's possible. And yes we should prevent people from adding dummy asserts to crash tests. |
See web-platform-tests/rfcs#33