[css-shapes] Refactor tests#19921
Conversation
foolip
left a comment
There was a problem hiding this comment.
My suggestions here are borderline what's reasonable to ask for on this cleanup PR, as the problems I point to were pre-existing. So please push back if you disagree.
| </div> | ||
| <div id="log"></div> | ||
| <script> | ||
| setup({ single_test: true }); |
There was a problem hiding this comment.
verifyTextPoints is synchronous, can you remove the done() call from it and just wrap this in test(() => {}) instead?
There was a problem hiding this comment.
Sure. As long as we're refactoring (and in keeping with the spirit of your suggestion), I've also moved the assertion into the test body.
There was a problem hiding this comment.
I think it would have been fine to leave it in the helper and that would have made the call sites look a bit simpler, but sure, there is a consistency to extracting that too.
| <script src="/resources/testharnessreport.js"></script> | ||
| <script src="support/spec-example-utils.js"></script> | ||
| <script> | ||
| setup({ single_test: true }); |
There was a problem hiding this comment.
approxShapeTest is also sync, but there's the onload="checkFloats();" bit below which makes the suggestion for verifyTextPoints unworkable. Instead, can you just pull the done() call into the test, after the approxShapeTest call?
356a548 to
59222a1
Compare
testharness.js was recently extended with an API to explicitly opt-in to the "single page test" feature [1]. As per WPT RFC 28 [2], tests which do not use this API and which do not declare any subtests will soon be reported as a harness error. Update some tests which previously opted in implicitly to use the new API. Update others to instead declare a single subtest (so that they are no longer single-page tests). [1] web-platform-tests#19449 [2] https://github.com/web-platform-tests/rfcs/blob/master/rfcs/single_test.md
59222a1 to
9168a23
Compare
|
Since the commit message would no longer describe this solution, I've amended the proposed commit and force-pushed the result to this branch. The original version of this branch is available here. |
This comment has been minimized.
This comment has been minimized.
|
My previous comment was intended for gh-19928. Sorry for the confusion! |
testharness.js was recently extended with an API to explicitly opt-in to
the "single page test" feature [1]. As per WPT RFC 28 [2], tests which
do not use this API and which do not declare any subtests will soon be
reported as a harness error.
Update some tests which previously opted in implicitly to use the new
API. Update others to instead declare a single subtest (so that they are
no longer single-page tests).
[1] #19449
[2] https://github.com/web-platform-tests/rfcs/blob/master/rfcs/single_test.md