Replace ava with vitest for frontend unit testing #8479
Conversation
📝 WalkthroughWalkthroughThis update migrates the frontend test suite from the AVA testing framework to Vitest, affecting numerous test files across the codebase. The migration replaces AVA-specific imports, test constructs, and assertion styles with Vitest equivalents. Mocking strategies are updated to utilize Vitest's mocking utilities ( Changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
For the most part, only the following changes needed to be done:
|
… reason. just to get the tests passing for now
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/build_test_deploy.yml (2)
44-48: Remove trailing spaces and consider cleaning up commented-out steps.There are trailing spaces at line 44, flagged by YAMLlint. Also, consider removing or documenting the commented-out artifact download step if it's not planned for future use, to keep the workflow clean.
- # - name: Download Coverage Artifacts - # uses: actions/download-artifact@v4 - # with: - # name: vitest-coverage-master - # path: coverage + # - name: Download Coverage Artifacts + # uses: actions/download-artifact@v4 + # with: + # name: vitest-coverage-master + # path: coverage[static_analysis]
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 44-44: trailing spaces
(trailing-spaces)
50-50: Fix excessive spaces after colon.Line 50 has too many spaces after the colon, which is flagged by YAMLlint. Adjust to a single space for YAML style consistency.
- uses: davelosert/vitest-coverage-report-action@v2 + uses: davelosert/vitest-coverage-report-action@v2[static_analysis]
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 50-50: too many spaces after colon
(colons)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build_test_deploy.yml(4 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build_test_deploy.yml
[error] 44-44: trailing spaces
(trailing-spaces)
[warning] 50-50: too many spaces after colon
(colons)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (3)
.github/workflows/build_test_deploy.yml (3)
31-31: Approve Node.js version update to 22.Upgrading to Node.js 22 across all jobs is a forward-compatible move and aligns with the updated frontend tooling requirements.
Also applies to: 71-71, 130-130
43-43: Approve migration to Vitest and coverage enablement.Switching the test command to use Vitest with coverage is correct and matches the new test suite setup.
49-59: Approve coverage reporting and artifact upload integration.Integrating Vitest coverage reporting and conditional artifact upload is a best practice for CI visibility and aligns with the new test stack.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 50-50: too many spaces after colon
(colons)
frontend/javascripts/oxalis/workers/byte_arrays_to_lz4_base64.worker.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/build_test_deploy.yml (2)
44-48: Remove trailing spaces and commented-out artifact download step for cleanliness.There are trailing spaces at line 44, and the commented-out artifact download step (lines 44-48) can be removed if not needed to keep the workflow clean and avoid linter warnings.
- # - name: Download Coverage Artifacts - # uses: actions/download-artifact@v4 - # with: - # name: vitest-coverage-master - # path: coverage🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 44-44: trailing spaces
(trailing-spaces)
50-50: Fix excessive spaces after colon for YAML style compliance.There are too many spaces after the colon on line 50. This is a minor YAML formatting issue flagged by static analysis.
- uses: davelosert/vitest-coverage-report-action@v2 + uses: davelosert/vitest-coverage-report-action@v2🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 50-50: too many spaces after colon
(colons)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/build_test_deploy.yml(4 hunks)frontend/javascripts/oxalis/workers/byte_array_lz4_compression.worker.ts(1 hunks)frontend/javascripts/oxalis/workers/byte_arrays_to_lz4_base64.worker.ts(1 hunks)frontend/javascripts/oxalis/workers/lz4_wasm_wrapper.ts(1 hunks)frontend/javascripts/test/global_mocks.ts(1 hunks)frontend/javascripts/test/model/binary/pullqueue.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- frontend/javascripts/oxalis/workers/byte_array_lz4_compression.worker.ts
- frontend/javascripts/oxalis/workers/lz4_wasm_wrapper.ts
- frontend/javascripts/oxalis/workers/byte_arrays_to_lz4_base64.worker.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/test/global_mocks.ts
- frontend/javascripts/test/model/binary/pullqueue.spec.ts
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/build_test_deploy.yml
[error] 44-44: trailing spaces
(trailing-spaces)
[warning] 50-50: too many spaces after colon
(colons)
🔇 Additional comments (2)
.github/workflows/build_test_deploy.yml (2)
31-31: Node.js version bump to 22 is appropriate.Upgrading to Node.js 22 aligns with the latest LTS and the requirements of modern tooling like Vitest. No issues found.
Also applies to: 71-71, 130-130
43-43: Frontend test command migration to Vitest is correct.Switching to
yarn run vitest run --coverage.enabled trueis the right approach for Vitest-based test suites and enables coverage reporting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
27-27: Minor grammar and spelling improvements for clarity.Consider updating the entry to read: "Replace frontend unit testing framework
avawithvitest. The minimum required Node.js version is now22+." This adds the missing determiner and uses the official spelling of "Node.js".Apply this diff for improved clarity:
- - Replace frontend unit testing framework `ava` with `vitest`. Minimum required nodejs version is now `22+`. [#8479](https://github.com/scalableminds/webknossos/pull/8479) + - Replace frontend unit testing framework `ava` with `vitest`. The minimum required Node.js version is now `22+`. [#8479](https://github.com/scalableminds/webknossos/pull/8479)🧰 Tools
🪛 LanguageTool
[uncategorized] ~27-~27: A determiner appears to be missing. Consider inserting it.
Context: ... testing frameworkavawithvitest. Minimum required nodejs version is now22+. [...(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~27-~27: The official spelling of this programming framework is “Node.js”.
Context: ...kavawithvitest. Minimum required nodejs version is now22+. [#8479](https://g...(NODE_JS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.unreleased.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[uncategorized] ~27-~27: A determiner appears to be missing. Consider inserting it.
Context: ... testing framework ava with vitest. Minimum required nodejs version is now 22+. [...
(AI_EN_LECTOR_MISSING_DETERMINER)
[uncategorized] ~27-~27: The official spelling of this programming framework is “Node.js”.
Context: ...k ava with vitest. Minimum required nodejs version is now 22+. [#8479](https://g...
(NODE_JS)
frontend/javascripts/test/sagas/volumetracing/bucket_eviction_helper.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/test/sagas/volumetracing/bucket_eviction_with_saving.spec.ts
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| // Mock common utility functions | ||
| // Mock modules instead of using mockRequire |
There was a problem hiding this comment.
let's not mention the you-know-who lib anymore ^^ shouldnt be relevant for future readers
| // Mock modules instead of using mockRequire |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
MIGRATIONS.unreleased.md (1)
11-11: Correct the official spelling of Node.js. The runtime should be referred to as “Node.js” (with a lowercase “j” and a period), not “NodeJs.”🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: The official spelling of this programming framework is “Node.js”.
Context: ...m//pull/8460) - NodeJs version22+is required. [#8479](http...(NODE_JS)
frontend/javascripts/test/helpers/apiHelpers.ts (3)
74-103: Consider adding JSDoc comments to mock implementation functionsThe implementation of
receiveJSONMockImplementationis clear but would benefit from JSDoc comments explaining what fixtures are expected and the overall purpose of this helper function.+/** + * Mock implementation for Request.receiveJSON + * Returns fixture data based on URL patterns and annotation type + * @param url The request URL + * @param options Request options + * @param annotationFixture The fixture data to use for annotation responses + * @returns Promise resolving to mock data + */ function receiveJSONMockImplementation( url: string, options: any, annotationFixture: typeof SKELETON_ANNOTATION | typeof VOLUME_ANNOTATION | typeof TASK_ANNOTATION, ) {
105-110: Consider typing the options parameterThe
_optionsparameter insendJSONReceiveJSONMockImplementationis marked with an underscore but typed asany. Consider creating a proper interface for this parameter or removing the underscore if the parameter is intentionally not used.-function sendJSONReceiveJSONMockImplementation(url: string, _options?: any) { +function sendJSONReceiveJSONMockImplementation(url: string, options?: RequestOptions) {Where
RequestOptionswould be a proper interface defined for this purpose.
222-223: Consider updating the error throwing patternThe current error handling creates a new Error with the message from the caught error, but ignores the stack trace from the original error.
- // @ts-ignore - throw new Error(error.message); + // Preserve the original error stack if possible + if (error instanceof Error) { + throw error; + } else { + throw new Error(String(error)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MIGRATIONS.unreleased.md(1 hunks)frontend/javascripts/test/global_mocks.ts(1 hunks)frontend/javascripts/test/helpers/apiHelpers.ts(4 hunks)frontend/javascripts/test/model/cuckoo_table_uint64.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/test/global_mocks.ts
- frontend/javascripts/test/model/cuckoo_table_uint64.spec.ts
🧰 Additional context used
🪛 LanguageTool
MIGRATIONS.unreleased.md
[uncategorized] ~11-~11: The official spelling of this programming framework is “Node.js”.
Context: ...m//pull/8460) - NodeJs version 22+ is required. [#8479](http...
(NODE_JS)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
- GitHub Check: build-smoketest-push
🔇 Additional comments (6)
frontend/javascripts/test/helpers/apiHelpers.ts (6)
1-1: Great modernization of imports for Vitest migrationThe imports have been properly updated to use Vitest types and testing utilities instead of the previous AVA/sinon setup. The explicit import of ModelType as type helps maintain type safety.
Also applies to: 25-36
45-54: Well-structured test context interfaceThe creation of the
WebknossosTestContextinterface extending the Vitest base context is a good practice. It provides strong typing for the test environment and makes the API contract between the test setup and test cases clear and maintainable.
56-68: Good implementation of Vitest mocking patternThe migration from manual mocks to using Vitest's
vi.mock()API is well-implemented. This centralizes the mocking logic and makes it more maintainable compared to the previous approach.
179-188: Well-structured test context populationGood job setting up the test context with all necessary components. The
tearDownPullQueueshelper is particularly useful for cleaning up resources between tests.
193-198: Good use of vi.mocked type assertionUsing
vi.mocked()to properly type the mocked functions is a good practice that ensures type safety when setting mock implementations.
206-225: Improved error handling with try/catchThe migration to async/await with proper try/catch handling is a significant improvement over the previous promise chain approach. The error reporting is clear and will make test failures easier to diagnose.
Co-authored-by: Philipp Otto <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/javascripts/test/helpers/apiHelpers.ts (1)
220-228: Simplify error handlingThe error handling logic can be simplified to avoid the ts-ignore and improve readability.
} catch (error) { console.error("model.fetch() failed", error); - if (error instanceof Error) { - throw error; - } else { - // @ts-ignore - throw new Error(error.message); - } + // Re-throw as Error with appropriate message + throw error instanceof Error ? error : new Error(String(error)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/test/helpers/apiHelpers.ts(4 hunks)
🧰 Additional context used
🪛 GitHub Check: frontend-tests
frontend/javascripts/test/helpers/apiHelpers.ts
[failure] 194-194:
Argument of type 'RequestOptions | undefined' is not assignable to parameter of type 'RequestOptions'.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (4)
frontend/javascripts/test/helpers/apiHelpers.ts (4)
1-39: Clean migration from ava to vitestThe imports have been successfully updated to use Vitest instead of ava for testing. The additional imports properly support the new asynchronous testing approach.
45-54: Well-structured test context interfaceThe new
WebknossosTestContextinterface is well-designed, extending Vitest'sBaseTestContextwith proper typing for model, mocks, API, and helper functions. This provides good type safety for tests.
56-144: Effective Vitest mocking implementationThe refactored mock implementations using Vitest's
vi.mock()and related utilities are correctly implemented. The mock implementations for Request and other modules provide appropriate test responses based on URL patterns.
169-188: Improved setup function with async/awaitThe renamed setup function with async/await pattern and proper test context initialization is a significant improvement over the previous implementation. Good job structuring the test context properties.
|
@philippotto Thanks for all the feedback. I addressed all of it. Ready to rumble. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/javascripts/test/helpers/apiHelpers.ts (1)
193-195:⚠️ Potential issueFix type error with RequestOptions parameter
The options parameter is passed directly to receiveJSONMockImplementation without checking if it's undefined, but the function expects a non-optional RequestOptions object.
vi.mocked(Request).receiveJSON.mockImplementation((url, options) => - receiveJSONMockImplementation(url, options, annotationFixture), + receiveJSONMockImplementation(url, options || {}, annotationFixture), );
🧹 Nitpick comments (1)
frontend/javascripts/test/helpers/apiHelpers.ts (1)
220-228: Improve error handling type safetyThe error handling uses @ts-ignore to work around a TypeScript error. It would be better to use a more type-safe approach to handle unknown error types.
try { // ...existing code... } catch (error) { console.error("model.fetch() failed", error); - if (error instanceof Error) { + // Handle all error types in a type-safe way + const errorMessage = error instanceof Error ? error.message : String(error); + throw new Error(`Failed to set up Webknossos for testing: ${errorMessage}`); - } else { - // @ts-ignore - throw new Error(error.message); - } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/test/helpers/apiHelpers.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/test/helpers/apiHelpers.ts (9)
frontend/javascripts/oxalis/model.ts (1)
ModelType(370-370)frontend/javascripts/oxalis/api/api_latest.ts (1)
ApiInterface(2823-2828)frontend/javascripts/libs/request.ts (1)
RequestOptions(32-32)frontend/javascripts/oxalis/model/actions/organization_actions.ts (1)
setActiveOrganizationAction(8-12)frontend/javascripts/oxalis/workers/slow_byte_array_lz4_compression.worker.ts (1)
setSlowCompression(10-12)frontend/javascripts/oxalis/model/helpers/proto_helpers.ts (2)
parseProtoTracing(23-38)parseProtoAnnotation(72-82)frontend/javascripts/oxalis/controller/scene_controller_provider.ts (1)
setSceneController(16-18)tools/proxy/proxy.js (1)
app(11-11)frontend/javascripts/oxalis/api/wk_dev.ts (1)
api(62-68)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (5)
frontend/javascripts/test/helpers/apiHelpers.ts (5)
46-54: Well-structured test context interfaceThe WebknossosTestContext interface is well-defined and provides clear typing for the test environment. This improves type safety and makes the test helpers more maintainable.
57-68: Clean and consistent mocking approachThe switch to Vitest's mocking facilities is clean and consistent. Using vi.mock() with specific mock implementations makes the test setup more explicit and easier to understand.
169-173: Improved function signature with async/awaitThe function signature has been improved by:
- Using a more descriptive name (setupWebknossosForTesting)
- Accepting a typed testContext parameter
- Using async/await for better readability and error handling
184-188: Good implementation of tearDownPullQueues helperThe tearDownPullQueues helper is a good addition that ensures tests clean up resources properly. This prevents test pollution and improves test isolation.
76-76: Good use of default parameter valueUsing a default empty object for the options parameter is a good practice to handle optional parameters safely.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/test/helpers/apiHelpers.ts (1)
206-228: Well-implemented async error handlingThe try-catch block with proper error logging and type checking for the Error instance follows best practices for async error handling in TypeScript.
However, consider simplifying the error re-throwing logic:
catch (error) { console.error("model.fetch() failed", error); - if (error instanceof Error) { - throw error; - } else { - // @ts-ignore - throw new Error(error.message); - } + // Re-throw as is if it's already an Error, otherwise wrap it + throw error instanceof Error ? error : new Error(String(error)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/test/helpers/apiHelpers.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/javascripts/test/helpers/apiHelpers.ts (7)
frontend/javascripts/oxalis/model.ts (1)
ModelType(370-370)frontend/javascripts/oxalis/api/api_latest.ts (1)
ApiInterface(2823-2828)frontend/javascripts/libs/request.ts (1)
RequestOptions(32-32)frontend/javascripts/oxalis/model/actions/organization_actions.ts (1)
setActiveOrganizationAction(8-12)frontend/javascripts/oxalis/workers/slow_byte_array_lz4_compression.worker.ts (1)
setSlowCompression(10-12)frontend/javascripts/oxalis/model/helpers/proto_helpers.ts (2)
parseProtoTracing(23-38)parseProtoAnnotation(72-82)frontend/javascripts/oxalis/controller/scene_controller_provider.ts (1)
setSceneController(16-18)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (10)
frontend/javascripts/test/helpers/apiHelpers.ts (10)
1-1: Good migration to Vitest testing frameworkThe import of Vitest's testing utilities and TestContext is appropriate for the migration from AVA.
25-25: Fixed import syntax for ModelTypeThe import syntax correctly uses a named import
{ ModelType }which matches the type export inoxalis/model.ts.
27-40: Well-organized imports for test setupThe imports are logically organized to support the test environment setup, including workers, models, and API interfaces needed for the testing functionality.
45-54: Good type safety with WebknossosTestContext interfaceThe extended test context interface improves type safety and provides better IDE support by explicitly defining the shape of the test context object.
56-72: Improved mocking approach with VitestUsing Vitest's
vi.mock()API for mocking modules is cleaner and more maintainable than the previous approach. This centralizes the mocking configuration and makes the testing setup more consistent.
74-77: Good fix for RequestOptions parameterMaking the
optionsparameter optional with a default empty object value fixes the type error identified in previous reviews.
112-122: Nicely implemented partial module mockingThe approach to import the original module and then override specific functions is a good pattern for partial mocking, preserving the original behavior for most of the module while only mocking what's needed for testing.
169-173: Improved function signature for setupWebknossosForTestingThe renamed function with async/await signature and strongly typed parameters is more descriptive and provides better TypeScript support.
179-188: Well-structured test context initializationThe initialization of the test context with model, mocks, and helper functions provides a clean API for tests to interact with the application.
197-198: Good use of lodash's cloneDeep for fixturesUsing
_.cloneDeep()ensures that each test gets fresh copies of the fixture data, preventing test interdependencies that could lead to flaky tests.
There was a problem hiding this comment.
Awesome!!! nicer code, easier to use and it's faster too (24 seconds / 22% faster when compared to another ci run).
thank you for tackling this issue!
|
With regards to speed: AFAIK, |
I tested this a bit last week with the sharding-feature of vitest. the feature allows to split all specs (within all files) into several shards and then run a vitest process for each shard. I did not notice any speed up (for some shard counts it got even worse). so, I assume that the tests are already cpu-bound. this was also the reason why I split the one big volume-related test suite into two. |
At the moment we have a a complicated and somewhat outdated stack for frontend testing:
esbuildnodejswhile the whole codebase already uses ES modulesmockRequireis also commonJS and very confusingnodejswithavaframeworkThis workflow for testing TS, ESM and React code in node is configured very differently from the regular browser builds and is not very well supported out-of-the-box by either
avaor the other tools.vitestis very modern, works out-of-the-box with our current frontend setup without any additional configs. n this PR I migrated the frontend unit tests tovitestin an effort to rid ourselves ofavaand manyesbuildconfigs.URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Update all remaining unit tests
Follow up issue: Convert all e2e tests to vitest
test/puppeteertest/screenshotstest/backend-snapshot-teststest/screenshots-wkorgIssues:
(Please delete unneeded items, merge only when none are left open)