Skip to content

test: [M3-6428] - Migrate from Jest to Vitest#9817

Merged
jdamore-linode merged 44 commits intolinode:developfrom
jdamore-linode:M3-6428-jest-to-vitest
Nov 16, 2023
Merged

test: [M3-6428] - Migrate from Jest to Vitest#9817
jdamore-linode merged 44 commits intolinode:developfrom
jdamore-linode:M3-6428-jest-to-vitest

Conversation

@jdamore-linode
Copy link
Contributor

@jdamore-linode jdamore-linode commented Oct 19, 2023

Description 📝

Replaces Jest with Vitest and attempts to get all of our tests passing. See also PR #8951 -- a lot of the test changes in this PR stem from that.

Trouble tests and other things to call attention to:

  • Made a change to confirmObjectStorage() in ObjectStorage/utilities.ts to convert the function to async/await syntax, but have not tested the change in Cloud
    • I think this is only called when enabling Object Storage by creating an access key
  • RescueDialog.test.tsx, SearchLanding.test.tsx, CreateAPITokenDrawer.test.tsx are each failing intermittently, but seem to pass when run by themselves
  • We get a lot of failures when running single threaded

I've kept pretty good documentation on each of the significant changes made to the tests, and plan to follow up with a self review that describes each significant change.

Changes 🔄

  • The majority of our tests pass by replacing jest with vi
  • Others required some minor refactoring, mostly involving module mocking and other calls to vi.mock, moving component rendering to inside of tests, and making changes involving async operations

How to test 🧪

yarn test

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@jdamore-linode jdamore-linode self-assigned this Oct 19, 2023
@jdamore-linode jdamore-linode requested a review from a team as a code owner October 19, 2023 14:11
@jdamore-linode jdamore-linode requested review from cliu-akamai and dwiley-akamai and removed request for a team October 19, 2023 14:11
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Going to circle back to review this more in-depth in the coming days, but this was my initial look at it. I ran yarn test and didn't get any failures 🎉 and it seems to run the test suite quite quickly

Screenshot 2023-10-27 at 12 09 46 PM

"start:manager:ci": "yarn workspace linode-manager start:ci",
"clean": "rm -rf node_modules && rm -rf packages/@linode/api-v4/node_modules && rm -rf packages/manager/node_modules && rm -rf packages/@linode/validation/node_modules",
"test": "yarn workspace linode-manager test --maxWorkers=4",
"test": "yarn workspace linode-manager test",
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we no longer have multiple workers running with the switch to Vitest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dwiley-akamai Nope! This just removes the upper limit of having 4 workers. If I remember correctly, Vitest (and maybe Jest too) will default to the number of available cores minus 1, but we added this argument to work around an issue we were having in CI, I believe.

I originally removed it to see if CI would pass without it, but I may restore it to see if having it improves performance at all. And if it doesn't, I'll remove it again 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no Vitest equivalent to jest.setTimeout, so in this case we just pass the desired timeout to the test as the third parameter when calling it(...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unlike Jest, when mocking modules, Vitest expects the entire module to be mocked. We accomplish this by importing the actual module and using it for the mock, overriding only the exports that we wish to mock.

Similar changes have been made throughout the codebase:

- src/features/Billing/BillingPanels/PaymentInfoPanel/PaymentInformation.test.tsx
- src/features/Billing/BillingPanels/BillingActivityPanel/BillingActivityPanel.test.tsx
- src/features/ObjectStorage/BucketDetail/ObjectDetailsDrawer.test.tsx
- src/features/Domains/DownloadDNSZoneFileButton.test.tsx
- src/components/PaymentMethodRow/PaymentMethodRow.test.tsx
- src/features/TopMenu/AddNewMenu/AddNewMenu.test.tsx
- src/features/Kubernetes/ClusterList/ClusterActionMenu.test.tsx
- src/features/StackScripts/StackScriptsLanding.test.tsx
- src/features/Linodes/LinodesDetail/LinodeBackup/LinodeBackups.test.tsx
- src/hooks/useOrder.test.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calls to jest.mock() and vi.mock() get hoisted to the top of the file during test transformation, so calling it inside of a test doesn't always result in the behavior we expect.

In some cases we want the mocked value to differ between tests in the same test file. Unlike Jest, Vitest offers a way for us to modify the mocked value using the vi.hoisted utility. We use this in SelectRegionPanel.test.tsx and RescueDialog.test.tsx.

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

🚀

Joe D'Amore and others added 2 commits November 2, 2023 14:14
Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com>
@coliu-akamai coliu-akamai self-requested a review November 2, 2023 18:54
@coliu-akamai
Copy link
Contributor

Coming over from frontend cafe 😆 -- will take a look tomorrow! 👀

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

Looks good, thanks Joe!! 🎉 Appreciate the explanations on this and the previous pr, was helpful for understanding the changes made

I did get the intermittent test failures you mentioned for SearchLanding.test.tsx and CreateAPITokenDrawer.test.tsx when running yarn test - any chance you know why they happen?

"build": "concurrently --raw \"tsc\" \"tsup\"",
"test": "jest",
"test:debug": "node --inspect-brk node_modules/.bin/jest --runInBand",
"test": "yarn vitest run",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we no longer need a test:debug script for vitest? 👀 (or was this script just unnecessary in the first place?)

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

I need to finish looking through the diff but I ran the tests a couple more times and got a handful of failures each time:

Screenshot 2023-11-06 at 4 43 45 PM

Screenshot 2023-11-06 at 4 49 38 PM

@jdamore-linode jdamore-linode merged commit 94f82bd into linode:develop Nov 16, 2023
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