Skip to content
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

[testharness.js] Require single-page test opt-in #20036

Merged

Conversation

jugglinmike
Copy link
Contributor

This enacts the change specified by WPT RFC 28.

https://github.com/web-platform-tests/rfcs/blob/master/rfcs/single_test.md


Overall, this patch appears to be safe to land, though we may want to wait for gh-19904 and gh-20031 before doing so.

In gh-19578, we aimed to extend all of WPT's single-page tests with the new implicit opt-in pattern. Once we completed that, I created this patch and ran it in Chrome Dev and Firefox Nightly.

Most of the discrepancies are due to flaky tests (listed below). The second most common cause comes from references to the API under test outside of a subtest (also listed below). In master today, the resulting ReferenceError causes the test to be reported as a failing single-page test, even though in most cases, it was authored to define multiple sub-tests. The new result is a harness error, and that's more appropriate for these cases.

There are a handful of tests whose results changed for other reasons:

  • these tests are being fixed via [WebCryptoAPI] Defer harness completion #19904
    • WebCryptoAPI/derive_bits_keys/
  • these tests rely on new ECMAScript language features that are not yet supported in Firefox. The harness error is a more appropriate status
    • IndexedDB/structured-clone.any.html
    • IndexedDB/structured-clone.any.worker.html
  • these tests includes an unresolvable reference. The harness error is a more appropriate status
    • fetch/metadata/sec-fetch-dest/redirect/multiple-redirect-https-downgrade-upgrade.tentative.sub.html
    • fetch/metadata/sec-fetch-dest/redirect/redirect-http-upgrade.tentative.sub.html
  • this test includes a syntax error. The harness error is a more appropriate status
    • fetch/metadata/sec-fetch-dest/redirect/redirect-https-downgrade.tentative.sub.html -
  • this test references an internal API which is not defined. The harness error is a more appropriate status
    • idle-detection/interceptor.https.html
  • these errors describe the intended effect of the patch
    • infrastructure/expected-fail/uncaught-exception.html
    • infrastructure/expected-fail/unhandled-rejection.html
  • these tests are single-page tests which we failed to identify initially; we're updating them via [preload] Opt-in to single-page test feature #20031
    • preload/preload-csp.sub.html
    • preload/preload-default-csp.sub.html
flaky tests
  • detected by Chrome
    • background-fetch/
    • content-security-policy/
    • css/ - flaky
    • html/semantics/links/links-created-by-a-and-area-elements/
    • intersection-observer/cross-origin-iframe.sub.html
    • longtask-timing/longtask-in-sibling-iframe-crossorigin.html
    • offscreen-canvas/
    • scroll-to-text-fragment/scroll-to-text-fragment.html
    • webaudio/the-audio-api/the-mediaelementaudiosourcenode-interface/cors-check.https.html
    • webmessaging/broadcastchannel/basics.html
    • webmessaging/broadcastchannel/workers.html
  • detected by Firefox
    • css/css-animations/CSSAnimation-startTime.tentative.html
    • css/css-backgrounds/background-334.html
    • css/css-multicol/animation/column-rule-color-interpolation.html
    • css/css-properties-values-api/property-cascade.html
    • css/css-transitions/events-007.html
    • css/css-writing-modes/text-combine-upright-parsing-valid-001.html
    • html/cross-origin-embedder-policy/require-corp.https.html
    • html/semantics/scripting-1/the-script-element/load-error-events-3.html
    • html/semantics/scripting-1/the-script-element/script-onerror-insertion-point-1.html
    • html/webappapis/dynamic-markup-insertion/opening-the-input-stream/abort-refresh-immediate.window.html
    • offscreen-canvas/the-offscreen-canvas/offscreencanvas.commit.w.html
    • performance-timeline/case-sensitivity.any.html
    • pointerevents/pointerevent_setpointercapture_inactive_button_mouse.html
    • referrer-policy/gen/req.attr/same-origin/img-tag/cross-https.swap-origin.http.html
    • resource-timing/resource_connection_reuse.html
    • resource-timing/resource_timing_buffer_full_eventually.html
    • service-workers/service-worker/ready.https.html
    • upgrade-insecure-requests/gen/worker-classic-data.meta/upgrade/fetch/cross-http-downgrade.downgrade.https.html
    • webaudio/the-audio-api/the-scriptprocessornode-interface/simple-input-output.html
    • webdriver/
    • webvtt/
unsafe API references
  • detected by Chrome
    • streams/readable-byte-streams/construct-byob-request.any.html
    • streams/readable-byte-streams/construct-byob-request.any.serviceworker.html
    • streams/readable-byte-streams/construct-byob-request.any.sharedworker.html
    • streams/readable-byte-streams/construct-byob-request.any.worker.html
    • trusted-types/
  • detected by Firefox
    • 2dcontext/wide-gamut-canvas/imageData-colorManagedBehavior.html
    • animation-worklet/worklet-animation-pause.https.html
    • animation-worklet/worklet-animation-without-target.https.html
    • css/css-properties-values-api/conditional-rules.html
    • css/css-properties-values-api/unit-cycles.html
    • css/css-properties-values-api/url-resolution.html
    • css/css-typed-om/
    • custom-elements/form-associated/ElementInternals-accessibility.html
    • custom-elements/form-associated/form-associated-callback.html
    • encoding/streams/decode-utf8.any.serviceworker.html
    • encoding/streams/realms.window.html
    • streams/piping/pipe-through.any.serviceworker.html
    • streams/readable-byte-streams/construct-byob-request.any.html
    • streams/readable-byte-streams/construct-byob-request.any.serviceworker.html
    • streams/readable-byte-streams/construct-byob-request.any.worker.html
    • streams/transform-streams/brand-checks.any.html
    • streams/transform-streams/brand-checks.any.serviceworker.html
    • streams/transform-streams/brand-checks.any.worker.html
    • streams/transform-streams/properties.any.serviceworker.html
    • streams/writable-streams/brand-checks.any.html
    • streams/writable-streams/brand-checks.any.serviceworker.html
    • streams/writable-streams/brand-checks.any.worker.html
    • streams/writable-streams/properties.any.serviceworker.html
    • trusted-types/GlobalEventHandlers-onclick.tentative.html
    • trusted-types/Node-multiple-arguments.tentative.html
    • trusted-types/TrustedTypePolicyFactory-metadata.tentative.html
    • trusted-types/WorkerGlobalScope-importScripts.https.html
    • trusted-types/block-Node-multiple-arguments.tentative.html
    • trusted-types/block-eval.tentative.html
    • trusted-types/block-string-assignment-to-Document-write.tentative.html
    • trusted-types/block-string-assignment-to-Element-setAttribute.tentative.html
    • trusted-types/eval-with-permissive-csp.tentative.html
    • trusted-types/trusted-types-eval-reporting-no-unsafe-eval.tentative.https.html
    • trusted-types/trusted-types-eval-reporting-report-only.tentative.https.html
    • trusted-types/trusted-types-eval-reporting.tentative.https.html
    • trusted-types/trusted-types-report-only.tentative.https.html
    • trusted-types/trusted-types-reporting-check-report.https.html
    • webaudio/the-audio-api/the-audioworklet-interface/audioworklet-messageport.https.html
    • webaudio/the-audio-api/the-audioworklet-interface/audioworklet-postmessage-sharedarraybuffer.https.html
    • webaudio/the-audio-api/the-audioworklet-interface/audioworkletglobalscope-sample-rate.https.html
    • webaudio/the-audio-api/the-audioworklet-interface/audioworkletglobalscope-timing-info.https.html
    • webaudio/the-audio-api/the-audioworklet-interface/audioworkletnode-onerror.https.html
    • webaudio/the-audio-api/the-audioworklet-interface/audioworkletnode-output-channel-count.https.html
    • webaudio/the-audio-api/the-mediaelementaudiosourcenode-interface/cors-check.https.html
    • webaudio/the-audio-api/the-mediaelementaudiosourcenode-interface/no-cors.https.html

@jugglinmike
Copy link
Contributor Author

gh-19904 and gh-20031 have both been merged

@foolip
Copy link
Member

foolip commented Nov 2, 2019

@jugglinmike I'll hold off until the tests pass on Azure Piplines and Taskcluster.

resources/testharness.js Show resolved Hide resolved
@jugglinmike
Copy link
Contributor Author

@jugglinmike I'll hold off until the tests pass on Azure Piplines and Taskcluster.

I've updated the relevant "functional" tests. Those could be extended to cover the new harness failure cases, but we're already testing all cases via two different approaches:

  • the infrastructure/ tests with expectations metadata
  • the testharness.js self-tests with harness instances nested in iframes

And since we've previously agreed that this third format is the most awkward to run and maintain, covering the new functionality yet again seems more like contributing to technical debt than preventing regressions.

@jugglinmike why aren't the tests.set_file_is_test() calls being removed, is this not the final PR in this process?

My mistake. The current behavior had me thinking that tests always report at least one subtest, but I was forgetting about harness TIMEOUT. It just comes down to calling tests.complete() directly instead of indirectly through done().

resources/testharness.js Outdated Show resolved Hide resolved
resources/testharness.js Outdated Show resolved Hide resolved
resources/testharness.js Outdated Show resolved Hide resolved
@foolip
Copy link
Member

foolip commented Nov 5, 2019

@jugglinmike expectations still have to be updated for some *.any.sharedworker.html tests in Safari, because the failure mode is now different.

@@ -792,7 +792,10 @@ policies and contribution forms [3].

function done() {
if (tests.tests.length === 0) {
tests.set_file_is_test();
tests.status.status = tests.status.ERROR;
tests.status.message = "done() was called without first defining any tests";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do any existing tests hit this in Chrome, Firefox or Safari?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My research above includes only Firefox and Chrome, but none of those discrepancies are due to an early done invocation (at least, not now that we've merged gh-19904).

This comment was marked as off-topic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sort of problem found in #20036 (comment) is what I had hoped to spot before this was merged.

@jugglinmike
Copy link
Contributor Author

All set, @foolip (GitHub was experiencing technical difficulties when I pushed the latest set of changes, so I needed to push an empty commit in order to trigger CI).

@foolip foolip merged commit f15a13d into web-platform-tests:master Nov 5, 2019
@foolip
Copy link
Member

foolip commented Nov 5, 2019

@jugglinmike merged!

It'd be good to compare the results of e09e55a and f15a13d when Taskcluster is finished running both, just to be sure this didn't affect any new tests since the last checking, unlikely as that is.

@foolip
Copy link
Member

foolip commented Nov 6, 2019

Checks comparing before/after results can be found at https://github.com/web-platform-tests/wpt/runs/290180050, and seq(status:!error status:error) can be used to look for new harness errors.

There are affected tests, enough that it seems worth checking carefully. web-platform-tests/wpt.fyi#462 would come in handy, but I think downloading the full reports and grepping in them is the only method I'd trust at this point. @jugglinmike can you check out if any of these changes are unintended?

Here's one that makes the failure mode less clear:
https://wpt.fyi/results/2dcontext/wide-gamut-canvas/imageData-colorManagedBehavior.html?diff&filter=ADC&run_id=350280003&run_id=350400002

@foolip
Copy link
Member

foolip commented Nov 6, 2019

Looks to me like the problem is that done() is used in error_handler and also in Test.prototype.cleanup, cases where the "done() was called without first defining any tests" error doesn't make sense. In the error_handler case it clobbers any previous error.

@foolip
Copy link
Member

foolip commented Nov 7, 2019

Ping @jugglinmike, #19993 is blocked on the above because some of the tests will result in ERROR instead of PRECONDITION_FAILED without a fix for this, and I don't want to sneak a fix directly into that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants