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

test: remove remaining vestiges of jest types #4435

Merged
merged 17 commits into from
Aug 7, 2024

Conversation

cardoso
Copy link
Contributor

@cardoso cardoso commented Aug 3, 2024

Details

I updated all jest type usage and declarations with equivalent the equivalent vitest counterparts.

I also fixed all errors vscode was reporting in test files: There's some conflicts with @types/mocha which seems to come from webdriver.io , so I had to add some explicit imports. I'm not sure there's a better way currently.

I also updated most comments and docs I felt confident were only referring to this project's tests. Some comments I just flagged to be updated as it might involve updating code I'm not familiar with (mainly lwc/default-exports).

Closes #4422

Does this pull request introduce a breaking change?

  • 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • 🤞 No, it does not introduce an observable change.

GUS work item

@cardoso cardoso requested a review from a team as a code owner August 3, 2024 23:55
Copy link
Contributor Author

@cardoso cardoso left a comment

Choose a reason for hiding this comment

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

Here's some things to address before merging

Comment on lines +91 to +94
if (
snapshotState['_updateSnapshot'] === 'new' ||
snapshotState['_updateSnapshot'] === 'all'
) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

_updateSnapshot is private in vitest which means there's probably a better way to do this, but for now this is needed to avoid annoying errors in vscode.

Comment on lines +114 to +122
import 'vitest';

interface CustomMatchers<R = unknown> {
toMatchFile: (receivedContent: string, filename?: string) => R;
}

declare module 'vitest' {
interface Assertion<T = any> extends CustomMatchers<T> {}
interface AsymmetricMatchersContaining extends CustomMatchers {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might fit better in a .d.ts file

Comment on lines 63 to 67
const realModule = await import(pathToEsmDistFile);
// The commend below needs to be updated:
// When jest properly supports ESM, this will be a lot simpler
// const aliasedModule = await import(`lwc/${pkg}`);
// expect(aliasedModule.default).toBe(realModule.default);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment and related code should be updated

}

expect.extend({
toBeInRange(code, min, max, key) {
toBeInRange(code, { min, max }, key) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typing will only work here if there's no more than 3 parameters, although tests would pass. VSCode shows errors otherwise.

@@ -31,7 +31,7 @@
"release:version": "./scripts/release/version.js"
},
"//": {
"prettier": "v3 requires ESM, and we use prettier in our Jest tests. Jest does not support ESM yet."
"prettier": "Outdated since Jest has been replaced with vitest: v3 requires ESM, and we use prettier in our Jest tests. Jest does not support ESM yet."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a reminder here, maybe for another PR? I reckon this is related to #4386

Copy link
Collaborator

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for this PR! LGTM, a few nits about explicit vs implicit globals.

I actually would prefer if we explicitly imported it/test/describe/etc. but since we're using globals for now, let's be consistent about it across all the test files.

ERROR_CODE_RANGES.compiler.max,
key
);
expect(errorInfo.code).toBeInRange(ERROR_CODE_RANGES.compiler, key);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice

packages/@lwc/shared/src/__tests__/html-attributes.spec.ts Outdated Show resolved Hide resolved
packages/lwc/__tests__/default-exports.spec.ts Outdated Show resolved Hide resolved
packages/lwc/__tests__/package-exports.spec.ts Outdated Show resolved Hide resolved
@nolanlawson
Copy link
Collaborator

/nucleus test

@nolanlawson nolanlawson requested a review from wjhsf August 6, 2024 17:51
import diff from 'jest-diff';
import MatcherUtils = jest.MatcherUtils;
import diff from '@vitest/utils/diff';
import type { MatcherState } from '@vitest/expect';
Copy link
Collaborator

@nolanlawson nolanlawson Aug 6, 2024

Choose a reason for hiding this comment

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

These packages both already come from vitest, but it makes me nervous not to have these explicitly declared in the root package.json. Let's add them.

@@ -5,11 +5,11 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

import diff from 'jest-diff';
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was getting pulled in by @wdio/cli! Whoops 🫣

@nolanlawson
Copy link
Collaborator

/nucleus test

@nolanlawson
Copy link
Collaborator

Sweet, thank you! Vitest all the things 💪

@nolanlawson nolanlawson merged commit 64ee414 into salesforce:master Aug 7, 2024
11 checks passed
@cardoso
Copy link
Contributor Author

cardoso commented Aug 13, 2024

@nolanlawson

I actually would prefer if we explicitly imported it/test/describe/etc. but since we're using globals for now, let's be consistent about it across all the test files.

Here's an attempt at a proper fix: #4458

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.

Remove remaining vestiges of jest types
2 participants