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: update test suites to leverage @web/test-runner and esm supported libraries #6847

Closed

Conversation

chrisdholt
Copy link
Member

Pull Request

📖 Description

This PR updates testing across the repo to leverage @web/test-runner. This is a necessary step to update TypeScript.

🎫 Issues

n/a

@chrisdholt chrisdholt force-pushed the users/chhol/convert-karma-tests-to-web-test-runner branch from e6638a6 to 1e9916f Compare September 30, 2023 10:01
@chrisdholt chrisdholt force-pushed the users/chhol/convert-karma-tests-to-web-test-runner branch 6 times, most recently from 94e3525 to 8bfdb06 Compare October 1, 2023 00:58
@chrisdholt chrisdholt force-pushed the users/chhol/convert-karma-tests-to-web-test-runner branch from 8bfdb06 to 39c0191 Compare October 11, 2023 20:40
run: |
yarn playwright install-deps
yarn playwright install
run: npx playwright install --with-deps
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has been a pain point for a while now - we're mixing NPM and Yarn across the repo and it's possible that this could result in conflicts since we're all using yarn locally, but the CI is using NPM. It makes it more complicated to debug when something goes wrong on the pipeline.

We should either be using only Yarn everywhere or only NPM everywhere, not both.

@@ -37,9 +37,8 @@
"watch": "yarn build -- -w --preserveWatchOutput"
},
"devDependencies": {
"@types/chai": "^4.2.11",
"@esm-bundle/chai": "4.3.4-fix.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a weird nonstandard version of a package that hasn't been published in over a year:

esm-bundle/chai#280
esm-bundle/chai#315

@@ -1,4 +1,4 @@
import chai from "chai";
import { expect } from "@esm-bundle/chai";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Chai version 5 is currently in alpha but is rewritten to only support ESM. Will we go back to Chai proper once it's stable?

https://github.com/chaijs/chai/releases/tag/v5.0.0-alpha.2

"test-chrome:verbose:debugger": "karma start karma.conf.cjs --browsers=ChromeDebugging --reporter=mocha",
"test-firefox": "karma start karma.conf.cjs --browsers=FirefoxHeadless --single-run --coverage",
"test-firefox:verbose": "karma start karma.conf.cjs --browsers=FirefoxHeadless --single-run --coverage --reporter=mocha"
"test": "yarn eslint && yarn build:tsc && web-test-runner && yarn doc:ci"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This script is run like a shell command - yarn takes care of it but other package managers might not in the future.

If we were to handle the transition from Yarn to NPM with a find/replace, it may turn into this:

$ npm run eslint
$ npm run build:tsc
$ web-test-runner # <-- this would break since `web-test-runner` isn't a direct command
$ npm run doc:ci

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, this could be handled with pretest and posttest scripts:

    "pretest": "yarn eslint && yarn build:tsc",
    "test": "web-test-runner",
    "posttest": "yarn doc:ci"

@@ -461,7 +459,7 @@ describe("The ElementController", () => {
controller.connect();
controller.disconnect();

expect(behavior.child.disconnectedCallback).to.have.been.called();
behavior.child.disconnectedCallback.called;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this statement value be asserted?

@@ -90,7 +90,7 @@ test.describe("Radio", () => {
`;
});

await expect(element).toHaveAttribute("tabindex", "");
await expect(element).not.toHaveAttribute("tabindex", "");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Playwright's toHaveAttribute doesn't properly handle the difference between a present attribute with a blank value (tabindex="") versus the non-existence of an attribute. This would be better expressed with hasAttribute:

await expect(element).not.hasAttribute("tabindex");

@chrisdholt chrisdholt force-pushed the users/chhol/convert-karma-tests-to-web-test-runner branch from 39c0191 to de59c54 Compare November 8, 2023 18:28
@chrisdholt chrisdholt force-pushed the users/chhol/convert-karma-tests-to-web-test-runner branch from de59c54 to 3ccdd58 Compare November 8, 2023 19:31
@radium-v
Copy link
Collaborator

@playwright/test should be upgradeable to the latest version (currently ^1.39.0) if yarn-deduplicate is used on the lockfile, to merge the different versions being pulled in.

(note: in my testing, @microsoft/fast-ssr will require locking [email protected] after using yarn-deduplicate!)

@radium-v
Copy link
Collaborator

When running all repo tests in streaming mode (yarn lerna run test --stream), there's a chance that runners will collide when trying to utilize port 8000. It's an intermittent failure which sometimes appears in parallel mode.

@janechu
Copy link
Collaborator

janechu commented May 30, 2024

Closing this as it has many conflicts and has been out of date for some time, additionally this will be resolved in a future PR after we remove yarn and switch to npm.

@janechu janechu closed this May 30, 2024
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.

5 participants