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

[core-util] Migrate from mocha & karma to vitest #27551

Merged
merged 25 commits into from
Dec 19, 2023
Merged

Conversation

xirzec
Copy link
Member

@xirzec xirzec commented Oct 25, 2023

Packages impacted by this PR

@azure/core-util

Describe the problem that is addressed by this PR

An experiment to migrate away from mocha and karma to vitest. This eliminates the need for a browser bundle as vitest uses the test source files directly.

Update: I have also created a small plugin to avoid needing to add additional browser mappings for testing

@xirzec xirzec self-assigned this Oct 25, 2023
@azure-sdk
Copy link
Collaborator

azure-sdk commented Oct 25, 2023

API change check

APIView has identified API level changes in this PR and created following API reviews.

@azure/core-util

"plugins": ["@azure/azure-sdk"],
"extends": ["plugin:@azure/azure-sdk/azure-sdk-base"],
"rules": {
"@azure/azure-sdk/ts-package-json-module": "off"
Copy link
Member Author

Choose a reason for hiding this comment

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

since we're not building tests anymore, this changed our dist-esm paths and the rule here doesn't account for it.

@xirzec xirzec marked this pull request as ready for review October 25, 2023 18:49
Comment on lines 8 to 13
browser: {
enabled: true,
headless: true,
name: "chromium",
provider: "playwright",
},
Copy link
Member

Choose a reason for hiding this comment

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

I wonder, could we disable security in the browser to get around CORS for testing like what we do now with karma or should we just bail on doing that?

Copy link
Member Author

Choose a reason for hiding this comment

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

There might be a way to pass those kinds of options, but do we have any SDKs currently that need that for their browser tests?

Copy link
Member

Choose a reason for hiding this comment

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

See here. IIRC, we do that to run AAD tests in the browser but perhaps we shouldn't?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm yeah I'm curious if this is needed for the kind of tests we normally do

Copy link
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

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

Although I wonder if there're cases where fine-grained control over bundling is needed, it is exciting that vitest abstracts a lot of the silly stuff needed to create those bundles so hopefully the majority of the libraries can migrate.

Also, could we vendor the testing command through dev-tool so we don't have to duplicate the config files and the dependencies in each library?

Copy link
Member

@jeremymeng jeremymeng left a comment

Choose a reason for hiding this comment

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

NICE!

sdk/core/core-util/vitest.browser.config.ts Outdated Show resolved Hide resolved
"downlevel-dts": "^0.10.0",
"cross-env": "^7.0.2",
"eslint": "^8.0.0",
"inherits": "^2.0.3",
"karma": "^6.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

👏

@xirzec xirzec requested a review from mikeharder as a code owner November 1, 2023 18:01
@xirzec xirzec requested a review from timovv as a code owner November 8, 2023 03:19
@xirzec xirzec requested a review from bterlson as a code owner December 4, 2023 20:46
@xirzec xirzec merged commit ed4eda1 into Azure:main Dec 19, 2023
13 checks passed
@xirzec xirzec deleted the vitest branch December 19, 2023 17:11
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