Migrate tests to use Vitest instead of jest by ngundotra · Pull Request #494 · solana-foundation/explorer

ngundotra · 2025-03-13T19:14:12Z

Description

Super annoying to setup tests for Squads tx inspector PR with jest because of ESM stuff. Vite just works

Type of change

Screenshots

Testing

pnpm test:ci

Related Issues

Checklist

All tests pass locally and in CI
CI/CD checks pass

Additional Notes

Important

Migrate testing framework from Jest to Vitest, updating test files, configurations, and dependencies accordingly.

Testing Framework Migration:
- Replace jest with vitest in all test files, including mocks.ts, endpoint.test.ts, fetch-resource.spec.ts, ip.spec.ts, and others.
- Update mock implementations from jest.mock to vi.mock.
- Remove Jest-specific files jest.config.js and jest.setup.js.
- Add test-setup.ts for Vitest setup.
- Update package.json scripts to use vitest for testing.
Configuration:
- Add vite.config.ts for Vitest configuration.
- Configure Vitest to use jsdom environment and v8 coverage provider.
- Set up path aliases in vite.config.ts.
Dependencies:
- Remove @swc/jest and jest from devDependencies.
- Add vitest, @vitejs/plugin-react, and @vitest/coverage-v8 to devDependencies.

^{This description was created by}^{for 5f688b5. It will automatically update as commits are pushed.}

vercel · 2025-03-13T19:14:16Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
explorer	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 18, 2025 5:26pm

ellipsis-dev

👍 Looks good to me! Incremental review on 74a7a54 in 1 minute and 55 seconds

More details

Looked at 29 lines of code in 1 files
Skipped 1 files when reviewing.
Skipped posting 6 drafted comments based on config settings.

1. package.json:13

Draft comment:
Using "vitest run" for CI is fine, but please confirm it behaves identically across environments compared to the previous "vitest --ci" flag.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50%
None

2. package.json:97

Draft comment:
Since Jest is being dropped, consider reviewing if 'jest-environment-jsdom' is still needed with Vitest. Ensure your Vitest config uses the proper test environment.
Reason this comment was not posted:
Comment was on unchanged code.

3. package.json:1

Draft comment:
The PR description states this resolves transaction history issues, yet the changes only involve test framework migration. Verify that tests or code changes addressing the transaction history bug are included elsewhere.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. package.json:13

Draft comment:
Confirm that 'vitest run' exits with a non-zero code on failures in CI.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
This comment is asking the PR author to confirm a behavior in CI, which is against the rules. It doesn't provide a specific code suggestion or ask for a test to be written. It seems to be asking for confirmation of behavior, which is not allowed.

5. package.json:80

Draft comment:
Ensure no lingering jest configuration remains after removing '@swc/jest' and 'jest'.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
The comment is asking the author to ensure that there are no leftover configurations after removing certain dependencies. This is a request for confirmation and not a specific code suggestion or issue identification. It violates the rule against asking the author to ensure something is done.

6. package.json:1

Draft comment:
PR description mentions resolving a transaction history bug, but the diff only migrates tests. Verify if the bug fix is included elsewhere.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_MsEwNoiCWIgPTita

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

…p… (#495) ## Description Add script to collect coverage ## Type of change - [x] Other (please describe): add coverage script ## Related Issues Relates to #494 ## Checklist - [x] My code follows the project's style guidelines - [ ] I have added tests that prove my fix/feature works - [x] All tests pass locally and in CI - [ ] I have updated documentation as needed - [x] CI/CD checks pass - [ ] I have included screenshots for protocol screens (if applicable) - [ ] For security-related features, I have included links to related information  ---- > [!IMPORTANT] > Add script and configuration for test coverage collection with atomic support using Vitest. > > - **Scripts**: > - Add `coverage` script in `package.json` to run `vitest --coverage.enabled`. > - **Dependencies**: > - Add `@vitest/coverage-v8` and `jsdom` to `devDependencies` in `package.json`. > - **Configuration**: > - Update `vite.config.ts` to enable atomic support in `poolOptions.threads.useAtomics` for tests. > > This description was created by [<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=solana-foundation%2Fexplorer&utm_source=github&utm_medium=referral) for b4347bb. It will automatically update as commits are pushed.  --------- Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>

ellipsis-dev

👍 Looks good to me! Incremental review on 5ed5797 in 1 minute and 26 seconds

More details

Looked at 129 lines of code in 5 files
Skipped 0 files when reviewing.
Skipped posting 13 drafted comments based on config settings.

1. app/__tests__/mocks.ts:8

Draft comment:
Ensure that replacing jest.mock with vi.mock works as expected for Next.js navigation mocks. The change looks correct, but consider adding a comment why ts-expect-error is needed for useSearchParams.mockReturnValue.
Reason this comment was not posted:
Confidence changes required: 20% <= threshold 50%
None

2. app/components/common/__tests__/AddressTableLookupAddress.spec.tsx:20

Draft comment:
Good migration: replacing jest.clearAllMocks with vi.clearAllMocks. Verify that all tests run as expected.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

3. app/components/common/__tests__/BaseInstructionCard.spec.tsx:19

Draft comment:
Test cleanup updated correctly with vi.clearAllMocks; migration seems seamless.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

4. app/components/inspector/__tests__/AssociatedTokenDetailsCard.spec.tsx:15

Draft comment:
Migration is correctly replacing jest.mock with vi.mock. Verify that all mock dependencies behave as expected under Vitest.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

5. app/components/instruction/__tests__/AssociatedTokenDetailsCard.spec.tsx:24

Draft comment:
Migration in this test file follows the pattern: using vi.clearAllMocks instead of jest.clearAllMocks and vi.mock for next/navigation. Everything looks appropriate.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

6. General:1

Draft comment:
The PR description mentions resolving an issue about transaction history for deleted accounts, but the changes in this PR only migrate tests from Jest to Vitest. Please ensure that this PR fully addresses the mentioned issue or that the changes are coordinated with other PRs.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. app/__tests__/mocks.ts:3

Draft comment:
Good migration: Importing vi from Vitest and using vi.mock instead of jest.mock is correctly done.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

8. app/components/common/__tests__/AddressTableLookupAddress.spec.tsx:4

Draft comment:
Importing vi and using vi.clearAllMocks in afterEach is correctly replacing Jest functions.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

9. app/components/common/__tests__/BaseInstructionCard.spec.tsx:4

Draft comment:
Migration to Vitest is complete; replacing jest.clearAllMocks with vi.clearAllMocks is properly handled.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

10. app/components/inspector/__tests__/AssociatedTokenDetailsCard.spec.tsx:15

Draft comment:
Using vi.mock for 'next/navigation' is correct and consistent with the migration.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

11. app/components/instruction/__tests__/AssociatedTokenDetailsCard.spec.tsx:6

Draft comment:
The tests correctly import vi from Vitest, effectively replacing Jest functions.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

12. app/components/instruction/__tests__/AssociatedTokenDetailsCard.spec.tsx:24

Draft comment:
Using vi.clearAllMocks for cleanup in afterEach is consistent with the new testing framework.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

13. GENERAL:0

Draft comment:
PR description references issue #123 concerning transaction history for deleted accounts, but the diff only shows test migration changes. Verify that the issue fix is included or update the PR description accordingly.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_2LF3x4NzHceLyKnf

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on 5f688b5 in 1 minute and 53 seconds

More details

Looked at 221 lines of code in 6 files
Skipped 0 files when reviewing.
Skipped posting 10 drafted comments based on config settings.

1. app/__tests__/mocks.ts:7

Draft comment:
Consider parameterizing the 'cluster' and 'customUrl' values instead of hardcoding them, so individual tests can override if needed.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50%
None

2. app/__tests__/mocks.ts:12

Draft comment:
Ensure proper return type for the mocked useSearchParams to avoid unexpected runtime issues.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50%
None

3. app/components/instruction/__tests__/AssociatedTokenDetailsCard.spec.tsx:18

Draft comment:
The @ts-expect-error comment indicates a type mismatch in mocking useSearchParams; consider declaring proper types so the error is not suppressed.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While the comment identifies a real TypeScript issue, this is a test file and mocking often requires type assertion or error suppression. The @ts-expect-error is a common and acceptable pattern in test files. The code works as intended, and adding proper types for mocks would add complexity without much benefit. The comment doesn't point to a critical issue that needs fixing.
The comment correctly identifies a type mismatch, and proper typing could make the code more maintainable. Test code quality is important too.
However, the benefit of adding proper types for test mocks is minimal compared to the effort required, and @ts-expect-error is a standard practice in test files.
The comment should be deleted as it suggests a change that would add complexity without significant benefit in a test context where type suppression is common and acceptable.

4. app/components/instruction/__tests__/ComputeBudgetDetailsCard.spec.tsx:17

Draft comment:
Verify that using 'devnet' as the mock return value for useSearchParams is intentional, as other tests default to 'mainnet-beta'.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50%
None

5. app/components/instruction/__tests__/ComputeBudgetDetailsCard.spec.tsx:28

Draft comment:
Check that the hardcoded Uint8Array data used to simulate ComputeBudget instructions remains valid with potential future changes to instruction formats.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50%
None

6. app/__tests__/mocks.ts:9

Draft comment:
Consider parameterizing the hardcoded values ('cluster' set to 'mainnet-beta' and 'customUrl' as undefined) to allow more flexible test scenarios.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The old version did have parameters which made it more flexible. The new version hardcodes these values which could make testing different scenarios harder. However, this is just a mock setup and the values may only be needed for this specific test case. Without seeing the tests that use this mock, we can't be certain that parameterization is actually needed.
I don't have visibility into how this mock is used across the test suite. The hardcoded values might be perfectly sufficient for all current test cases.
While parameterization could add flexibility, we should only suggest changes when we have strong evidence they're needed. Making the mock more complex without a clear need violates YAGNI principles.
Without seeing the tests that use this mock, we can't be confident that parameterization is necessary. The comment should be removed as it's speculative.

7. app/components/common/__tests__/BaseInstructionCard.spec.tsx:10

Draft comment:
Removal of beforeEach/afterEach hooks (used for clearing mocks) is acceptable if Vitest is configured to clear mocks between tests. Ensure that mock call histories are properly reset to maintain test isolation.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
This comment is purely informative, explaining the conditions under which the removal of beforeEach/afterEach hooks is acceptable. It does not provide a specific suggestion or ask for a specific confirmation related to the code changes. It also includes a general instruction to ensure mock call histories are reset, which is not specific to the code changes in the PR.

8. app/components/instruction/__tests__/AssociatedTokenDetailsCard.spec.tsx:17

Draft comment:
Avoid duplicate mocking of 'next/navigation'. A global mock is defined in mocks.ts; consolidating these mocks can help prevent potential conflicts and improve maintainability.
Reason this comment was not posted:
Comment was on unchanged code.

9. app/components/instruction/__tests__/ComputeBudgetDetailsCard.spec.tsx:15

Draft comment:
Overriding the useSearchParams mock value to return 'devnet' is intentional for this test scenario. Just ensure this override doesn’t conflict with tests expecting 'mainnet-beta'.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

10. General:1

Draft comment:
The PR description mentions resolving issue 123 (transaction history for deleted accounts not accessible), but the diff only contains test migration changes. Please ensure that functionality regarding deleted accounts is adequately covered by tests.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_Hn7wEjxtLsXibZjR

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

rogaldh · 2025-03-17T20:37:15Z

I think that --watch flag is not needed at the test script. Vitest applies --watch by default. Also it does respect CI=true env which is present at GithubActions. That means that we can use pnpm t everywhere:

locally it will apply --watch mode
at CI watch mode will be ignored

package.json

rogaldh

Other than things above LGTM

#497) ## Description Update config for vitest to disable deprecated usage of CJS implementation of fetch. Changed deprecated format for deps also. ## Type of change  - [x] Other (please describe): fix deprecations ## Testing `pnpm t` You won't wee deprecation warnings from vitest ## Related Issues Relates to #494 ## Checklist  - [x] My code follows the project's style guidelines - [x] I have added tests that prove my fix/feature works - [x] All tests pass locally and in CI - [x] I have updated documentation as needed - [x] CI/CD checks pass - [ ] I have included screenshots for protocol screens (if applicable) - [ ] For security-related features, I have included links to related information  ---- > [!IMPORTANT] > Update `vite.config.mts` to remove deprecation warnings by moving `deps` under `server`. > > - **Configuration Update**: > - Renamed `vite.config.ts` to `vite.config.mts`. > - Moved `deps` configuration under `server` in `vite.config.mts` to address deprecation warnings related to CJS fetch implementation. > - **Testing**: > - Running `pnpm t` will no longer show deprecation warnings from vitest. > > This description was created by [<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=solana-foundation%2Fexplorer&utm_source=github&utm_medium=referral) for beafd40. It will automatically update as commits are pushed.

ngundotra force-pushed the vitest branch from 3139c3d to 74a7a54 Compare March 13, 2025 19:15

ellipsis-dev bot reviewed Mar 13, 2025

View reviewed changes

vercel bot deployed to Preview March 13, 2025 19:18 View deployment

rogaldh mentioned this pull request Mar 14, 2025

chore: add script to collect coverage with atomics support to speed up… #495

Merged

8 tasks

ngundotra added 6 commits March 17, 2025 09:57

add testing lib

6eb4f93

migrate ed25519 test

b92cf9d

migrate lighthouse test

af7a38b

migrate utils test

3979833

add vite config

7fc56fb

migrate featureGate

7079468

vercel bot deployed to Preview March 17, 2025 16:58 View deployment

ngundotra and others added 9 commits March 17, 2025 09:58

migrate computebudget test

f2d576a

polyfill & migrate fetch-resource

d8582f6

finish feature-gate migrate

212ba69

migrate endpoint & ip test

b2fea46

remove unused import

f9c31fb

remove jest configs

34822b0

fix ci command and remove unused packages

2681e56

rebase

5ed5797

ngundotra force-pushed the vitest branch from 8d19508 to 5ed5797 Compare March 17, 2025 17:04

ellipsis-dev bot reviewed Mar 17, 2025

View reviewed changes

vercel bot deployed to Preview March 17, 2025 17:06 View deployment

fix router mock

5f688b5

vercel bot deployed to Preview March 17, 2025 18:24 View deployment

ellipsis-dev bot reviewed Mar 17, 2025

View reviewed changes

rogaldh reviewed Mar 17, 2025

View reviewed changes

package.json Show resolved Hide resolved

rogaldh mentioned this pull request Mar 17, 2025

chore: update vite config to remove deplrecation warnings with CJS fe… #497

Merged

8 tasks

rogaldh approved these changes Mar 17, 2025

View reviewed changes

vercel bot deployed to Preview March 18, 2025 17:26 View deployment

ngundotra merged commit f10cc58 into master Mar 18, 2025
4 checks passed

ngundotra deleted the vitest branch March 18, 2025 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate tests to use Vitest instead of jest#494

Migrate tests to use Vitest instead of jest#494
ngundotra merged 17 commits intomasterfrom
vitest

ngundotra commented Mar 13, 2025 •

edited by ellipsis-dev bot

Loading

Uh oh!

vercel bot commented Mar 13, 2025 •

edited

Loading

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

rogaldh commented Mar 17, 2025

Uh oh!

Uh oh!

rogaldh left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ngundotra commented Mar 13, 2025 • edited by ellipsis-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Screenshots

Testing

Related Issues

Checklist

Additional Notes

Uh oh!

vercel bot commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

rogaldh commented Mar 17, 2025

Uh oh!

Uh oh!

rogaldh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ngundotra commented Mar 13, 2025 •

edited by ellipsis-dev bot

Loading

vercel bot commented Mar 13, 2025 •

edited

Loading