chore(test): add initial WebdriverIO infrastructure #5456

christian-bromann · 2024-03-11T19:40:11Z

What is the current behavior?

The team has decided to press forward with WebdriverIO as the replacement for Karma/Browserstack

In order to make it such that the team can swarm on replacing these ~150 tests, we need some initial infrastructure in place.

What is the new behavior?

This patch adds initial infrastructure to run WebdriverIO tests locally and in CI.

Documentation

Adding some docs on how to migrate tests.

Does this introduce a breaking change?

Yes
No

Testing

I assume as long as the pipeline passes, we should be good.

Other information

STENCIL-1190

github-actions · 2024-03-11T19:41:43Z

`--strictNullChecks` error report

Typechecking with --strictNullChecks resulted in 1145 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/compiler/transpile/transpile-module.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/mock-doc/test/attribute.spec.ts	12

Our most common errors

Typescript Error Code	Count
TS2322	361
TS2345	349
TS18048	207
TS18047	82
TS2722	37
TS2532	24
TS2531	21
TS2454	14
TS2790	11
TS2352	10
TS2769	8
TS2538	8
TS2416	6
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

github-actions · 2024-03-11T19:41:52Z

PR built and packed!

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

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

unzip -d ~/Downloads ~/Downloads/stencil-core-4.12.6-dev.1710355064.cea2cfb.tgz.zip && npm install ~/Downloads/stencil-core-4.12.6-dev.1710355064.cea2cfb.tgz

rwaskiewicz

I took a first pass at this PR - I looked at most of the infra here and had some questions about how it's running today/how we might shrink this diff a bit. I haven't looked at the tests themselves in a ton of detail yet - I'll take a second pass (including the tests) once we resolve the first round of questions - LMK if I can add any more detail!

.github/workflows/test-wdio.yml

rwaskiewicz · 2024-03-12T12:09:37Z

.github/workflows/test-wdio.yml

+  build_core:
+    name: Build
+    uses: ./.github/workflows/build.yml
+


No action required right now, but we could make this a little more similar to test-unit where we have a reusable workflow that uses the build_core build from the main workflow, instead of something separate. That'd give us a little less to manage here in terms of infra-as-code, but that's about it

.github/workflows/test-wdio.yml

test/karma/package-lock.json

test/wdio/README.md

rwaskiewicz · 2024-03-12T12:46:01Z

test/karma/test-app/components.d.ts

@@ -5,49 +5,11 @@
 * It contains typing information for all components that exist in this project.
 */
 import { HTMLStencilElement, JSXBase } from "@stencil/core/internal";
-import { SomeTypes } from "./util";


As we look to swarm on these, we might need to do a little extra coordination around this file in particular - I can see it being a source of frustration when it comes to rebasing, others landing changes, etc., since it changes anytime a component is removed/altered here.

It probably doesn't make sense to keep a version of each component file in karma, as we'd lose git history (or, we couldn't preserve it with git mv...I think) but maybe I'm wrong there.

Maybe the best thing we can do is to just keep conversion cycles tight - but I'm open to thoughts from @ionic-team/stencil here

Just tying this comment back to this thread.

For posterity, we're gonna ignore the components.d.ts file in the wdio dir, but can't do that ATM for the Karma tests.

test/karma/test-app/attribute-basic/karma.spec.ts

.github/workflows/test-wdio.yml

test/wdio/package.json

alicewriteswrongs

tried this out locally and it works great! looks like CI is working too

I just had one question, which I don't think is blocking, about how it makes sense for us to set up the CI workflow stuff

excited to get to converting the rest of these tests!

alicewriteswrongs · 2024-03-12T18:08:25Z

.github/workflows/test-wdio.yml

+      - name: Run WebdriverIO Component Tests
+        run: npm run test.wdio


I think it's sort of six of one half a dozen of another but would it make sense for us to kick off the wdio tests from within the main workflow, like we do for the unit tests and whatnot? instead of having a separate workflow here that runs independently

also is the plan to circle back later to do some sort of matrix on browsers and browser versions?

to kick off the wdio tests from within the main workflow, like we do for the unit tests and whatnot?

The test run is cheap and can be run as part of the unit test.

also is the plan to circle back later to do some sort of matrix on browsers and browser versions?

Just pushed a fix to include Firefox and Safari when running in CI

I recognize that there are some browser differences with some assertions. I had to change some assertion values based on whether we run in Safari or not. This could be either an actual browser issue or something with the driver underneath. I would like to investigate this separately. This could become ultimately a burden to maintain at some point. I can bring this up at the next Jam session.

In the first commit we move over all files we've initially migrated during the POC into the new wdio directory. This allows to keep the history of these files. STENCIL-1190

christian-bromann · 2024-03-13T03:20:01Z

I had to bring back test/karma/test-app/components.d.ts as it is required for the Karma test application to build. I don't see that this could pose a problem to us given the fact that we mostly just remove lines from that file.

test/wdio/attribute-boolean/cmp.test.tsx

rwaskiewicz · 2024-03-12T21:10:30Z

test/wdio/attribute-boolean/cmp.test.tsx

+    await expect($(root)).toHaveAttribute('aria-hidden', 'true');
+    await expect(root).toHaveAttribute('fixedtrue', 'true');
+    await expect(root).toHaveAttribute('fixedfalse', 'false');
+    await expect(root).toHaveAttribute('readonly', 'true');


It looks like this value changed from '' to 'true' between Karma and WDIO. Looking at the assertion immediately following this one (await expect(root).toHaveAttribute('tappable', '');), the value we use there is an empty string.

Is there any significance between using '' vs 'true' when using the toHaveAttribute matcher? If so, why do we need to set this value to 'true' for this assertion?

The readonly property is a boolean value which can be set in the following ways:

<input type="text" readonly /> <input type="text" readonly="" /> <input type="text" readonly="readonly" /> <input type="text" readonly="ReAdOnLy" />

The WebDriver protocol transforms this property into its actual value (e.g. true) while calling getAttribute just returns the value set to the property (e.g. ''). This is different from e.g. tappable which is not a reserved HTML property and can contain arbitrary strings.

That said, it is worth investigating constraints using reserved attributes on Stencil components. One could definitely expect to have it work like any custom property, especially if applied to elements where readonly has no effect on (e.g. text, search, url, tel, email, password, date, month, week, time, datetime-local, and number types and the <textarea> form control elements).

test/wdio/attribute-complex/cmp.test.tsx

test/wdio/attribute-host/cmp.test.tsx

rwaskiewicz · 2024-03-13T13:08:48Z

test/wdio/computed-properties-watch-decorator/cmp.test.tsx

+
+  it('triggers the watch callback when the associated prop changes', async () => {
+    const el = document.querySelector('computed-properties-watch-decorator');
+    await expect(el).toHaveText(['First name called with: not yet', 'Last name called with: not yet'].join('\n'));


In the Karma version of the test (where we didn't use toHaveText):

expect(el.innerText).toBe('First name called with: not yet\n\nLast name called with: not yet');

I noticed the double \n in the old test, whereas we have a single one in this test. I'm guessing this has to do with how toHaveText operates (I'm guessing not on innerText, perhaps textContent?), is that the reason for this (tiny) difference?

FWIW, the rendered component looks correct to me, which is why I assume this is just an API difference, but I could be wrong!

The WebDriver "get element test" command returns a representation of the "visible" state of a text. While it is not possible to define this concretely, they are relying on an implementation for it that has been used for many years. This implementation seems to remove double \n as they are not impacting how the text is rendered. innerText on the other hand actually gives you the characters of the DOM node which doesn't contains any filtering.

test/wdio/conditional-rerender/cmp.test.tsx

test/wdio/image-import/cmp.test.tsx

rwaskiewicz · 2024-03-13T13:30:35Z

test/wdio/stencil.config.ts

+    },
+  ],
+  plugins: [sass()],
+  buildDist: true,


Comparing this to karma/stencil.config.ts, it looks like we're not bringing buildEs5 over to this config. I'm OK with that, but wanted to make sure I understand what the motivation for that was (I assume it's speed/warning related, but wanted to make sure I'm not missing anything).

It seemed to me that this option generates more output that is not relevant to these tests so I left it out. Frankly, I'm not entirely certain about the specific differences it entails.

rwaskiewicz · 2024-03-13T13:37:54Z

test/karma/test-app/components.d.ts

@@ -5,49 +5,11 @@
 * It contains typing information for all components that exist in this project.
 */
 import { HTMLStencilElement, JSXBase } from "@stencil/core/internal";
-import { SomeTypes } from "./util";


Just tying this comment back to this thread.

For posterity, we're gonna ignore the components.d.ts file in the wdio dir, but can't do that ATM for the Karma tests.

tanner-reits

Couple questions, nothing major

.github/workflows/test-wdio.yml

test/wdio/attribute-complex/cmp.test.tsx

test/wdio/tsconfig-stencil.json

christian-bromann · 2024-03-13T18:38:25Z

@rwaskiewicz @tanner-reits thanks for the feedback. I addressed the comments.

tanner-reits

One more question, non-blocking

tanner-reits · 2024-03-13T19:50:24Z

test/wdio/computed-properties-prop-decorator/cmp.test.tsx

  beforeEach(async () => {
    render({
      template: () => (
-        <div>
+        <>


Should Fragment be in this tag?

Isn't this done automatically?

christian-bromann force-pushed the cb/karma-webdriverio-replace branch from 6b8d30f to 6d17f26 Compare March 11, 2024 23:58

christian-bromann marked this pull request as ready for review March 11, 2024 23:59

christian-bromann requested a review from a team as a code owner March 11, 2024 23:59

christian-bromann requested review from rwaskiewicz and tanner-reits March 11, 2024 23:59

christian-bromann mentioned this pull request Mar 12, 2024

[Tracking] WebdriverIO Migration #5458

Closed

93 tasks

rwaskiewicz suggested changes Mar 12, 2024

View reviewed changes

christian-bromann requested a review from rwaskiewicz March 12, 2024 17:24

alicewriteswrongs approved these changes Mar 12, 2024

View reviewed changes

christian-bromann added 5 commits March 12, 2024 15:16

chore(test): add initial WebdriverIO infrastructure

876d6dc

In the first commit we move over all files we've initially migrated during the POC into the new wdio directory. This allows to keep the history of these files. STENCIL-1190

rename karma files into cmp.test.tsx

2c9a2fd

rewrite tests into WebdriverIO

f1e33f9

remove html files

ae803a4

fix prettier

f621cf6

christian-bromann force-pushed the cb/karma-webdriverio-replace branch from 6d0e6c3 to f621cf6 Compare March 12, 2024 22:32

christian-bromann added 10 commits March 12, 2024 15:41

run with more browser in CI

abb786f

update comment

857520f

prettier

9e33b00

cross browser compat

e8fc640

prettier

fc43713

fix for chrome

96cfe6e

fix assertion

3500143

run in Edge instead of Safari

f6edc7b

prettier

88b2707

bring back components.d.ts

e264886

rwaskiewicz suggested changes Mar 13, 2024

View reviewed changes

tanner-reits requested changes Mar 13, 2024

View reviewed changes

.github/workflows/test-wdio.yml Outdated Show resolved Hide resolved

test/wdio/attribute-complex/cmp.test.tsx Outdated Show resolved Hide resolved

test/wdio/attribute-complex/cmp.test.tsx Outdated Show resolved Hide resolved

test/wdio/tsconfig-stencil.json Outdated Show resolved Hide resolved

PR feedback

60af735

christian-bromann requested review from tanner-reits and rwaskiewicz March 13, 2024 18:38

tanner-reits approved these changes Mar 13, 2024

View reviewed changes

rwaskiewicz approved these changes Mar 13, 2024

View reviewed changes

christian-bromann added this pull request to the merge queue Mar 13, 2024

Merged via the queue into main with commit 908f778 Mar 13, 2024
123 checks passed

christian-bromann deleted the cb/karma-webdriverio-replace branch March 13, 2024 23:49

rwaskiewicz added the WDIO Migration label Mar 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(test): add initial WebdriverIO infrastructure #5456

chore(test): add initial WebdriverIO infrastructure #5456

christian-bromann commented Mar 11, 2024

github-actions bot commented Mar 11, 2024

github-actions bot commented Mar 11, 2024 •

edited

Loading

rwaskiewicz left a comment

rwaskiewicz Mar 12, 2024

rwaskiewicz Mar 12, 2024

rwaskiewicz Mar 13, 2024

alicewriteswrongs left a comment

alicewriteswrongs Mar 12, 2024

alicewriteswrongs Mar 12, 2024

christian-bromann Mar 12, 2024

christian-bromann Mar 12, 2024

christian-bromann commented Mar 13, 2024

rwaskiewicz Mar 12, 2024

christian-bromann Mar 13, 2024

rwaskiewicz Mar 13, 2024

christian-bromann Mar 13, 2024 •

edited

Loading

rwaskiewicz Mar 13, 2024

christian-bromann Mar 13, 2024

rwaskiewicz Mar 13, 2024

tanner-reits left a comment

christian-bromann commented Mar 13, 2024

tanner-reits left a comment

tanner-reits Mar 13, 2024

christian-bromann Mar 13, 2024

		- name: Run WebdriverIO Component Tests
		run: npm run test.wdio

chore(test): add initial WebdriverIO infrastructure #5456

chore(test): add initial WebdriverIO infrastructure #5456

Conversation

christian-bromann commented Mar 11, 2024

What is the current behavior?

What is the new behavior?

Documentation

Does this introduce a breaking change?

Testing

Other information

github-actions bot commented Mar 11, 2024

--strictNullChecks error report

reports and statistics

Unused exports report

github-actions bot commented Mar 11, 2024 • edited Loading

PR built and packed!

rwaskiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christian-bromann commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christian-bromann Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanner-reits left a comment

Choose a reason for hiding this comment

christian-bromann commented Mar 13, 2024

tanner-reits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`--strictNullChecks` error report

github-actions bot commented Mar 11, 2024 •

edited

Loading

christian-bromann Mar 13, 2024 •

edited

Loading