Skip to content

chore(test): Minor tweaks to migrated WebdriverIO tests#5498

Merged
christian-bromann merged 5 commits into
mainfrom
cb/wdio-test-fixes
Mar 21, 2024
Merged

chore(test): Minor tweaks to migrated WebdriverIO tests#5498
christian-bromann merged 5 commits into
mainfrom
cb/wdio-test-fixes

Conversation

@christian-bromann

Copy link
Copy Markdown
Member

What is the current behavior?

I saw some weird behavior when reviewing #5478. We moved forward and merged the PR achieving the same assertions in a different way, however I went back and fixed some things in WebdriverIO to get it working.

What is the new behavior?

The way @alicewriteswrongs original expected WebdriverIO to work, works now. I updated the tests to reflect this. I also fixed some linting issues (mostly import orders).

Documentation

n/a

Does this introduce a breaking change?

  • Yes
  • No

Testing

n/a

Other information

n/a

@github-actions

github-actions Bot commented Mar 16, 2024

Copy link
Copy Markdown
Contributor

--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

@github-actions

github-actions Bot commented Mar 16, 2024

Copy link
Copy Markdown
Contributor

PR built and packed!

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

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

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

@rwaskiewicz rwaskiewicz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we update the commit msg + PR title to be chore(test):? While this fixes some things for our tests internally, it won't have an affect on end users & shouldn't show up in the autogenerated changelog

Comment thread test/wdio/lifecycle-basic/cmp-a.test.tsx Outdated
Comment thread test/wdio/package.json Outdated

@alicewriteswrongs alicewriteswrongs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good, other than @rwaskiewicz's comment

@christian-bromann

Copy link
Copy Markdown
Member Author

Blocked on webdriverio/webdriverio#12528

prettier

minor test tweaks

update deps

more fixes

@christian-bromann christian-bromann left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Some comments on particular patterns I picked up:

wrapping into browser.waitUntil

if certain sequences are uncertain due to their async nature, it might make sense to wrap them in a waitUntil block which re-executes the function until the condition is met. It can be used to use querySelector and attempt to fetch an element until found, e.g.:

const elem: HTMLElement = await browser.waitUntil(async () => {
  return document.body.querySelector('form');
});

Here if document.body.querySelector('form') doesn't find an element in the first attempt, it retries until it gets there. Consequently if the assertion fails for some reason the block will be retried as well. If you don't return a value, make sure to return a truthy value at the end so the condition is noted as successful.

Querying/Asserting Children

We have some tests where we would do the following:

    const host = document.querySelector('import-aliasing');

    expect(host.children[0].textContent).toBe('My name is John');
    expect(host.children[1].textContent).toBe('Name changed 0 time(s)');

@rwaskiewicz found a practical way of getting ahold of the children using WebdriverIO primitives by using a > * at the end of the query, e.g.:

    const children = $$('import-aliasing > *');
    await expect(children[0]).toHaveText('My name is John');
    await expect(children[1]).toHaveText('Name changed 0 time(s)');

However I will add a children() command to WebdriverIO so we can just do:

    const children = $('import-aliasing').children();
    await expect(children[0]).toHaveText('My name is John');
    await expect(children[1]).toHaveText('Name changed 0 time(s)');

Note: I've never considered this really a usecase as WebdriverIO was primarily used for e2e testing where you usually would query an element straight rather than going through the DOM tree. With component and unit testing in the browser this becomes more crucial.

Retries

I think this is one of the key points that makes the test more stable. While one could say, retries are an antipattern, in our case, it is cheap to do and has a big impact. I really can't say in some cases why certain tests are unreliable. A lot is executing very fast which I believe can also impact the browser rendering or other components we don't have control over.

retrigger build

retrigger build

retrigger build
rwaskiewicz added a commit that referenced this pull request Mar 21, 2024
configure renovate to group all `@wdio/` scoped npm packages and the
`expect-webdriverio` package into a single pr when renovate detects
version bumps.

the impetus for this work was a comment made during the wdio migration,
available [here](#5498 (comment))

STENCIL-1191

@rwaskiewicz rwaskiewicz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good - only ask before we merge is to update the PR title/commit msg #5498 (review)

@christian-bromann christian-bromann changed the title fix(test): Minor tweaks to migrated WebdriverIO tests chore(test): Minor tweaks to migrated WebdriverIO tests Mar 21, 2024
@christian-bromann christian-bromann added this pull request to the merge queue Mar 21, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Mar 21, 2024
@christian-bromann christian-bromann added this pull request to the merge queue Mar 21, 2024
Merged via the queue into main with commit be50d32 Mar 21, 2024
@christian-bromann christian-bromann deleted the cb/wdio-test-fixes branch March 21, 2024 19:28
github-merge-queue Bot pushed a commit that referenced this pull request Mar 21, 2024
configure renovate to group all `@wdio/` scoped npm packages and the
`expect-webdriverio` package into a single pr when renovate detects
version bumps.

the impetus for this work was a comment made during the wdio migration,
available [here](#5498 (comment))

STENCIL-1191
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.

3 participants