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

Update web-platform tests #3203

Merged
merged 30 commits into from Jun 15, 2021
Merged

Update web-platform tests #3203

merged 30 commits into from Jun 15, 2021

Conversation

ninevra
Copy link
Contributor

@ninevra ninevra commented Jun 11, 2021

Updates web-platform-tests, per #3200 (comment).

Let me know if I missed anything or did anything terribly wrong.

Interesting new test failures (with the caveat that I really don't know what I'm looking at or talking about here):

  • html/browsers/the-window-object:
    • window-indexed-properties-delete-no-cache.html: in jsdom, when the window has iframes, the numeric properties that refer to them (e.g. window[0]) can be deleted. Deletion should fail.
  • xhr:
    • formdata/constructor-formelement.html: formdata should normalize linebreaks to \n, but does not?
  • html/semantics/forms/the-textarea-element:
    • wrapping-transformation.window.html: TextArea normalizes newlines for the purpose of the value getter, but not the textContent getter?
  • html/semantics/embedded-content/the-img-element:
    • img-picture-ancestor.html, source-media-outside-doc.html: Img elements don't set their currentSrc in some cases; possibly related to media queries?
  • html/syntax:
    • xmldecl/xmldecl-*.html: looks like the content encoding of xml documents is being detected wrong?
  • shadow-dom:
    • imperative-slot-api*: tests for a new API (Slot.assign()) that jsdom doesn't implement yet?

Should issues be opened for any of the above?

@TimothyGu
Copy link
Member

TimothyGu commented Jun 11, 2021

Thanks a lot for the hard work in triaging the new tests!

  • html/browsers/the-window-object:

    • window-indexed-properties-delete-no-cache.html: in jsdom, when the window has iframes, the numeric properties that refer to them (e.g. window[0]) can be deleted. Deletion should fail.

This is probably hard to fix, as the deletion behavior is specified by WindowProxy's [[Delete]] abstract operation, which we can't override without making WindowProxy a Proxy object (which is hard). (We also can't make numeric properties non-configurable, since we need to be able to delete the properties ourselves when an iframe gets detached.)

  • xhr:

    • formdata/constructor-formelement.html: formdata should normalize linebreaks to \n, but does not?

This probably has to do with whatwg/html#6287, part of https://blog.whatwg.org/newline-normalizations-in-form-submission. We probably have to adjust our own code to conform.

  • html/semantics/forms/the-textarea-element:

    • wrapping-transformation.window.html: TextArea normalizes newlines for the purpose of the value getter, but not the textContent getter?

Probably related to the FormData test above. The spec now says "The algorithm for obtaining the element's API value is to return the element's raw value, with newlines normalized." The value getter returns the element's API value.

  • html/semantics/embedded-content/the-img-element:

    • img-picture-ancestor.html, source-media-outside-doc.html: Img elements don't set their currentSrc in some cases; possibly related to media queries?

Not sure. We don't support media queries particularly well.

  • html/syntax:

    • xmldecl/xmldecl-*.html: looks like the content encoding of xml documents is being detected wrong?

This is probably https://github.com/whatwg/html/pull/1752/files, which creates the get an XML encoding algorithm. That'll have to be fixed in https://github.com/jsdom/html-encoding-sniffer.

  • shadow-dom:

    • imperative-slot-api*: tests for a new API (Slot.assign()) that jsdom doesn't implement yet?

Let's create an issue for this.


Should issues be opened for any of the above?

Usually we fix simple issues in the same PR as the WPT roll. Unless the test is too hard to enable, in which case we just add the expectations in the to-run YAML file.

Copy link
Member

@TimothyGu TimothyGu left a comment

Looks good for the most part.

test/web-platform-tests/to-run.yaml Outdated Show resolved Hide resolved
@ninevra
Copy link
Contributor Author

ninevra commented Jun 14, 2021

It looks like the tests are completing successfully in about 25 minutes, but not exiting, causing CI to time out after 6 hours.

@ninevra
Copy link
Contributor Author

ninevra commented Jun 14, 2021

Implemented the FormData and TextArea changes; newlines are no longer normalized to CRLF, since the spec changes deferred that process to the actual encoding step. Implemented the textarea wrapping transformation, wrapping textarea content to at most cols characters per line, not including newlines. Re-enabled the two corresponding tests.

@TimothyGu
Copy link
Member

TimothyGu commented Jun 14, 2021

It looks like the tests are completing successfully in about 25 minutes, but not exiting, causing CI to time out after 6 hours.

Could you reproduce this timeout locally? If so, you could try using why-is-node-running or wtfnode to help debug what's preventing the process from exiting.

See the comment at start-wpt-server.js lines 56-57.
@ninevra
Copy link
Contributor Author

ninevra commented Jun 15, 2021

It looks like the tests are completing successfully in about 25 minutes, but not exiting, causing CI to time out after 6 hours.

Could you reproduce this timeout locally? If so, you could try using why-is-node-running or wtfnode to help debug what's preventing the process from exiting.

I can reproduce the timeout locally. I'm messing around with wtfnode and haven't learned anything conclusive yet. After running yarn test, there are two start-wpt-server.js child processes, presumably one for tuwpt and one for wpt, and five sockets connected to port 9000. I'll keep looking into this if/when I have time.

However, in the process I did find a strange inconsistency and possible solution:

process.on("exit", () => {
// Python doesn't register a default handler for SIGTERM and it doesn't run __exit__() methods of context
// managers when it gets that signal. Using SIGINT avoids this problem.
subprocess.kill("SIGINT");
});
notes that SIGINT is necessary to kill the test server, but the after() hook in
after(() => {
serverProcess.kill();
});
instead uses the default, SIGTERM, which the test server presumably ignores.

Changing the latter to SIGINT appears to cause the test server to exit normally.

@ninevra
Copy link
Contributor Author

ninevra commented Jun 15, 2021

The remaining CI failures occur in a new test that uses globalThis, which doesn't exist on node 10. Could solve this by:

  • have jsdom define globalThis in the vm even if it doesn't exist in the node environment;
  • add a mechanism for setting test expectations conditionally on the node version;
  • add a mechanism for expecting either pass or fail from a given subtest;
  • mark the whole test fail-slow;
  • drop Node 10, given it's EOL

Thoughts?

@domenic
Copy link
Member

domenic commented Jun 15, 2021

Dropping Node 10 is planned; you can proceed assuming this will get merged after #3192

@TimothyGu
Copy link
Member

TimothyGu commented Jun 15, 2021

@ninevra We do have the capability of marking the test as conditional on a Node.js version through needs-node10. Or we can wait it out, like @domenic mentioned :)

Both check on globalThis, which is only present on node 12+.
@TimothyGu TimothyGu merged commit 346ea98 into jsdom:master Jun 15, 2021
7 checks passed
@ninevra
Copy link
Contributor Author

ninevra commented Jun 17, 2021

Thanks @TimothyGu for all the help & guidance on this PR, the pointers to the various specs were really helpful and I'm glad I was able to contribute!

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

Successfully merging this pull request may close these issues.

None yet

3 participants