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

Migrate the custom element polyfill tests to @web/test-runner. #509

Merged
merged 112 commits into from
Aug 11, 2022

Conversation

bicknellr
Copy link
Collaborator

@bicknellr bicknellr commented Apr 25, 2022

This PR migrates the custom element polyfill tests from WCT to @web/test-runner.

  • The existing scoped custom elements polyfill tests were already using WTR, but this PR does not merge them into the tests package.
  • WTR and related dependencies have been moved from the root to the individual packages that actually depend on them (tests and scoped-custom-element-registry).
  • The tests package now includes a plugin for @web/dev-server that matches enough of the compilation behavior of polyserve to provide the tests with the same environment as before.
  • The GitHub Actions jobs for WCT and WTR tests have been merged.

This also removes the tests for the scoped custom element registry from those
run during the root `test` npm script. These tests are currently known to be
failing and were not previously run during automated tests.
…. (...)

These tests are currently failing, but were not being run before.
`outerHTML` is not wrapped by the polyfill and these tests were not being run.
@bicknellr bicknellr requested review from aomarks and rictic June 28, 2022 23:56
@bicknellr bicknellr marked this pull request as ready for review June 28, 2022 23:56
@bicknellr
Copy link
Collaborator Author

This still depends on getting the issue linked in the description fixed and I'd like to factor the WTR configs a bit differently, but I think this PR is close enough to start getting some feedback. PTAL, thanks!

@bicknellr bicknellr removed the request for review from aomarks June 28, 2022 23:58
@bicknellr bicknellr changed the title Migrate the tests to @web/test-runner. Migrate the custom element polyfill tests to @web/test-runner. Jun 28, 2022
Copy link
Contributor

@rictic rictic left a comment

Choose a reason for hiding this comment

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

packages/tests/package.json Show resolved Hide resolved
packages/tests/shadydom/event-path.html Show resolved Hide resolved
packages/tests/web-test-runner.config.js Show resolved Hide resolved
packages/tests/webcomponentsjs_/wct-config.js Show resolved Hide resolved
Copy link
Collaborator Author

@bicknellr bicknellr left a comment

Choose a reason for hiding this comment

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

BTW those links are to the same run :) That particular error is due to modernweb-dev/web#1986. I've been locally testing this PR on Safari 9 and 10 on Sauce by modifying files my tests package's node_modules to match the PR I pushed for that issue. Getting that fixed is definitely a blocker.

Also, that test in the event-path.html file where I added done() and one other in that same file fail in at least Chrome 41 when Shady DOM's noPatch mode is enabled. Though, I don't think these should necessarily be considered a blocker for this PR since they're legit failures AFAICT.

packages/tests/webcomponentsjs_/wct-config.js Show resolved Hide resolved
packages/tests/web-test-runner.config.js Show resolved Hide resolved
@@ -10,214 +10,109 @@
-->
<title>Custom Elements Tests</title>
<meta charset="utf-8" />
<script src="../../node_modules/@webcomponents/webcomponentsjs/bundles/webcomponents-pf_js.js"></script>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to double check that adding this was actually necessary. I don't remember if I did this before or after I had the test environment script in a reasonable state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems like this was necessary. The custom elements polyfill depends on Promise support (so every other test file already includes it) but this one originally didn't because it's for the native shim. The test environment now also depends on promises.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it would be more proper for the test environment to include a promise polyfill itself and for this one import to be removed, but I'm getting a different set of errors in IE 11 with core-js' promise polyfill added. I'll keep trying for a little bit - maybe I've chosen an entry point that assumes other features that aren't available in IE 11.

packages/tests/package.json Show resolved Hide resolved
@bicknellr bicknellr requested a review from aomarks as a code owner July 19, 2022 00:05
@bicknellr
Copy link
Collaborator Author

Now that modernweb-dev/web#1993 has landed and those components were released in modernweb-dev/web#1997, the proper way to update this would be to wait for a release of @web/test-runner-saucelabs that depends on at least @web/test-runner-webdriver@0.5.1 and have the tests package depend on at least that version. In the mean time, I've added @web/test-runner-webdriver@^0.5.1 as a dependency of the tests package, which is compatible with @web/test-runner-saucelabs' dependency/constraint of @web/test-runner-webdriver@^0.5.0, so @web/test-runner-webdriver@0.5.1 will be installed first under tests and then @web/test-runner-saucelabs will have its dependency of @web/test-runner-webdriver@^0.5.0 deduped against it, causing it to use 0.5.1.

@bicknellr
Copy link
Collaborator Author

Opened modernweb-dev/web#1999 to bump @web/test-runner-saucelabs' dependency range for @web/test-runner-webdriver to ^0.5.1.

@rictic
Copy link
Contributor

rictic commented Jul 22, 2022

Quoting the changelog presubmit:

Please add a CHANGELOG entry, or add the "Skip Changelog" label if not required.

Copy link
Contributor

@rictic rictic left a comment

Choose a reason for hiding this comment

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

Looks like tests are still failing on Sauce

@bicknellr
Copy link
Collaborator Author

I added the label because this PR has no user-facing changes. Also, those failures on Saucelabs are real failures from before this PR. In packages/tests/shadydom/event-path.html, the "composedPath of events from non-Node subclasses works" now fails because of the change I made here that causes the test to recognize an error thrown before the test ends. I think the other, "focus currentTarget is valid when unwrapped in noPatch", was already failing before but wasn't caught because we couldn't get the tests to run reliably and decided to rely on the internal tests instead.

@bicknellr bicknellr self-assigned this Aug 3, 2022
…. (...)

`@web/test-runner-saucelabs@0.8.1` is the first version that depends on
`@web/test-runner-webdriver@0.5.1`, which contains updates required by this
branch.
@bicknellr bicknellr requested a review from rictic August 10, 2022 22:35
Copy link
Contributor

@rictic rictic left a comment

Choose a reason for hiding this comment

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

Great work!

@bicknellr bicknellr merged commit b266e8a into master Aug 11, 2022
@bicknellr bicknellr deleted the convert-to-wtr branch August 11, 2022 00:34
@bicknellr
Copy link
Collaborator Author

Thanks for the taking the time to review!

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.

2 participants