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

css/css-animations/keyframes-remove-documentElement-crash.html causes a harness failure #20020

Closed
graouts opened this issue Oct 31, 2019 · 2 comments · Fixed by #21052
Closed

Comments

@graouts
Copy link
Contributor

graouts commented Oct 31, 2019

The test at css/css-animations/keyframes-remove-documentElement-crash.html causes a harness failure because it removes the <body> element:

[Error] TypeError: null is not an object (evaluating 'root.appendChild')
	(anonymous function) (testharness.js:2987)
	(anonymous function) (testharness.js:3026)
	(anonymous function) (testharness.js:196)
	(anonymous function) (testharness.js:2776)
	forEach (testharness.js:3489)
	(anonymous function) (testharness.js:2773)
	all_complete (testharness.js:2643)
	(anonymous function) (testharness.js:2655)
	(anonymous function) (testharness.js:2446)

I'm not sure what the right way would be to address this given the test design. It does seem somewhat arbitrary that this test exists in the WPT suite though.

Tagging the author of the test @lilles, its reviewer @mstensho, other Blink developers working on Web Animations @stephenmcgruer @flackr and all-around knowledgeable person @birtles.

@graouts graouts changed the title css/css-animations/event-order.tentative.html causes a harness failure css/css-animations/keyframes-remove-documentElement-crash.html causes a harness failure Nov 1, 2019
@stephenmcgruer
Copy link
Contributor

Interestingly, it seems to work ok under wpt run (I presume, based on https://wpt.fyi/results/css/css-animations/keyframes-remove-documentElement-crash.html?label=master&label=experimental&aligned&q=css%2Fcss-animations%2Fkeyframes-remove-documentelement-crash.html).

I think it's worth taking a quick look at it from the infra side, to understand what the difference is there.

In terms of whether the test 'belongs' in WPT, we've historically chosen to accept tests like this as they test valid spec behavior without being browser specific. That is, any implementation which failed this test would be against spec.

@foolip
Copy link
Member

foolip commented Nov 6, 2019

web-platform-tests/rfcs#33 is adding proper support for crash tests in WPT and then this could probably be rewritten to not use testharness.js and thus avoid the problem entirely.

There are differences between ./wpt run and navigating to a test on wpt.live, but exactly what the relevant difference is here I don't know.

@stephenmcgruer stephenmcgruer self-assigned this Nov 11, 2019
stephenmcgruer added a commit that referenced this issue Jan 6, 2020
…ness.js

Previously this test would fail when loaded locally as the
testharness.js cleanup code would try to access
document.documentElement, which had been removed. Instead, use the
crashtest support
(https://web-platform-tests.org/writing-tests/crashtest.html).

Fixes #20020
stephenmcgruer added a commit that referenced this issue Jan 6, 2020
…ness.js

Previously this test would fail when loaded locally as the
testharness.js cleanup code would try to access
document.documentElement, which had been removed. Instead, use the
crashtest support
(https://web-platform-tests.org/writing-tests/crashtest.html).

Fixes #20020
stephenmcgruer added a commit that referenced this issue Jan 6, 2020
…ness.js

Previously this test would fail when loaded locally as the
testharness.js cleanup code would try to access
document.documentElement, which had been removed. Instead, use the
crashtest support
(https://web-platform-tests.org/writing-tests/crashtest.html).

Fixes #20020
stephenmcgruer added a commit that referenced this issue Jan 15, 2020
…ness.js (#21052)

Previously this test would fail when loaded locally as the
testharness.js cleanup code would try to access
document.documentElement, which had been removed. Instead, use the
crashtest support
(https://web-platform-tests.org/writing-tests/crashtest.html).

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

Successfully merging a pull request may close this issue.

3 participants