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

chore(test): migrate Karma tests to WebdriverIO #5465

Merged
merged 16 commits into from
Mar 26, 2024

Conversation

christian-bromann
Copy link
Member

Copy link
Contributor

github-actions bot commented Mar 14, 2024

--strictNullChecks error report

Typechecking with --strictNullChecks resulted in 1143 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files
Path Error Count
src/dev-server/index.ts 37
src/dev-server/server-process.ts 32
src/compiler/prerender/prerender-main.ts 22
src/testing/puppeteer/puppeteer-element.ts 22
src/runtime/client-hydrate.ts 20
src/screenshot/connector-base.ts 19
src/runtime/vdom/vdom-render.ts 17
src/dev-server/request-handler.ts 15
src/compiler/prerender/prerender-optimize.ts 14
src/compiler/sys/stencil-sys.ts 14
src/sys/node/node-sys.ts 14
src/compiler/prerender/prerender-queue.ts 13
src/compiler/sys/in-memory-fs.ts 13
src/runtime/connected-callback.ts 13
src/runtime/set-value.ts 13
src/compiler/output-targets/output-www.ts 12
src/compiler/transformers/test/parse-vdom.spec.ts 12
src/compiler/transformers/transform-utils.ts 12
src/compiler/transpile/transpile-module.ts 12
src/mock-doc/test/attribute.spec.ts 12
Our most common errors
Typescript Error Code Count
TS2322 362
TS2345 346
TS18048 204
TS18047 82
TS2722 37
TS2532 24
TS2531 21
TS2454 14
TS2790 11
TS2352 11
TS2769 8
TS2416 8
TS2538 8
TS2493 3
TS18046 2
TS2684 1
TS2430 1

Unused exports report

There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!

Unused exports
File Line Identifier
src/runtime/bootstrap-lazy.ts 21 setNonce
src/screenshot/screenshot-fs.ts 18 readScreenshotData
src/testing/testing-utils.ts 198 withSilentWarn
src/utils/index.ts 145 CUSTOM
src/utils/index.ts 269 normalize
src/utils/index.ts 7 escapeRegExpSpecialCharacters
src/compiler/app-core/app-data.ts 25 BUILD
src/compiler/app-core/app-data.ts 115 Env
src/compiler/app-core/app-data.ts 117 NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts 123 updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts 61 satisfies
src/compiler/types/validate-primary-package-output-target.ts 61 Record
src/testing/puppeteer/puppeteer-declarations.ts 485 WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts 7 writeFetchSuccessSync

Copy link
Contributor

github-actions bot commented Mar 14, 2024

PR built and packed!

Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/8443380194/artifacts/1361072641

If your browser saves files to ~/Downloads you can install it like so:

unzip -d ~/Downloads ~/Downloads/stencil-core-4.13.0-dev.1711490124.e02344d.tgz.zip && npm install ~/Downloads/stencil-core-4.13.0-dev.1711490124.e02344d.tgz

const { WWW_OUT_DIR } = require('../../constants');

module.exports = {
entry: path.resolve(__dirname, 'index.esm.js'),
Copy link
Member Author

Choose a reason for hiding this comment

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

Does someone know the point of having Webpack involved here? It doesn't seem to do anything other than re-bundling the package and importing it in the browser. I believe we cover this feature with the setup script automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @rwaskiewicz is familiar with the basis for the setup -- if it's possible for us to do without it I'm all for it because it's super confusing haha

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, reason for this was two-fold:

  1. to test Stencil could go through the webpack process (this is a regression Stencil had at some point)
  2. To use the dist-custom-elements output target, as opposed to www

@christian-bromann
Copy link
Member Author

@rwaskiewicz @tanner-reits any objections to merge?

@tanner-reits
Copy link
Contributor

@christian-bromann looks like there are some CI failures

@christian-bromann
Copy link
Member Author

looks like there are some CI failures

Addressed them, they sneaked it after the last rebase

Copy link
Contributor

@rwaskiewicz rwaskiewicz left a comment

Choose a reason for hiding this comment

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

For the most part, the changes look good here!

Some of these tests were built specifically for DCE or Webpack regressions/features - with that in mind, I think there's some value in bringing them back.

Previously we kinda hacked this in as a 2 part build - we'd have build.app kick off a Stencil Build that included dist-custom-elements, then we'd have webpack bundle/move the files to the www output directory (build.custom-elements job) so they'd get kicked off by the test runner.

It's all a bit confusing TBH, let me know if you wanna jam on ways we can put this together to be a little more cohesive 😆

@christian-bromann
Copy link
Member Author

Some of these tests were built specifically for DCE or Webpack regressions/features - with that in mind, I think there's some value in bringing them back.

@rwaskiewicz I tracked down the source of the test which was introduced in #3039. From what I can see is that this verifies that the DCE output target provides a defineCustomElement method to register the custom element. I would suggest to tweak the setup script to not run for this test and have the component imported and registered manually and verify that the component can be initialized properly.

I don't think it makes sense anymore to keep Webpack as a dependency for this test stack.

@rwaskiewicz
Copy link
Contributor

@christian-bromann

I would suggest to tweak the setup script to not run for this test and have the component imported and registered manually and verify that the component can be initialized properly.

I don't think it makes sense anymore to keep Webpack as a dependency for this test stack.

I think I'd be amenable to that 👍

@christian-bromann
Copy link
Member Author

I think I'd be amenable to that 👍

I made changes accordingly. Let me know what you think.

Copy link
Contributor

@rwaskiewicz rwaskiewicz left a comment

Choose a reason for hiding this comment

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

One non-blocking question

Comment on lines +134 to +148
await browser.waitUntil(async () => {
return document.querySelector('div.reordered');
});

reordered();

await $('button').click();
await browser.waitUntil(() => {
await browser.waitUntil(async () => {
return !document.querySelector('div.reordered');
});

ordered();

await $('button').click();
await browser.waitUntil(() => {
await browser.waitUntil(async () => {
Copy link
Contributor

@rwaskiewicz rwaskiewicz Mar 26, 2024

Choose a reason for hiding this comment

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

Does passing an async function to waitUntil do anything special? Or do we not need async on these fns?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've seen race conditions saying .then is not a function and my assumptions are that this has to do with WebdriverIO. It is on the list of things I'ld like to look into eventually. In the meeting, I am adding this to avoid the race conditions.

@christian-bromann christian-bromann added this pull request to the merge queue Mar 26, 2024
Merged via the queue into main with commit d8ab583 Mar 26, 2024
123 checks passed
@christian-bromann christian-bromann deleted the cb/test-migration branch March 26, 2024 22:08
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.

4 participants