Skip to content

test(ai): migrate tests to Vitest - #10363

Merged
Manvi1203 merged 8 commits into
mainfrom
feature/vitest-ai
Sep 28, 2026
Merged

Manvi1203 merged 8 commits into
mainfrom
feature/vitest-ai

Conversation

@Manvi1203

@Manvi1203 Manvi1203 commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Description

Migrates @firebase/ai unit tests from legacy Karma & Mocha to Vitest (Node + Browser Chromium).

Key Changes

  • Vitest Runner: Added vitest.config.mjs extending config/vitest.base.mjs and removed deprecated karma.conf.js.
  • Test Scripts: Standardized npm test scripts to pure vitest commands (test:all, test:node, test:browser).
  • ESM & Browser Fixes:
    • Replaced Node-specific global references with standard ECMAScript globalThis in live-session-helpers.test.ts.
    • Replaced Mocha this.skip() and describe early returns with conditional runner skips (describe.skip / it.skip) in chrome-adapter.test.ts.
    • Handled Sinon fake timer asynchronous promise rejections before clock.tickAsync in request.test.ts.
    • Added mock WebSocket constants and bounded stream polling in websocket.test.ts.
    • Configured vi.mock('...', { spy: true }) with vi.spyOn(...) and vi.resetAllMocks() in test/setup.ts for native ESM browser execution across generative model and chat session tests.
  • Type Cleanup: Separated type-only exports in src/api.ts and src/types/index.ts.
  • Ambient Types: Added test/types/vitest-globals.d.ts for isolated test typings.

Testing & Impact

  • Execution Time: Dropped from ~15s (Karma + Mocha) to 6.4s (Vitest).

@Manvi1203
Manvi1203 requested a review from a team as a code owner September 8, 2026 19:29
@changeset-bot

changeset-bot Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 3addc62

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@firebase/ai Patch
firebase Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Vertex AI Mock Responses Check ⚠️

A newer major version of the mock responses for Vertex AI unit tests is available. update_vertexai_responses.sh should be updated to clone the latest version of the responses: v17.0

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request migrates the test suite for the packages/ai package from Karma and Mocha to Vitest, which includes updating package scripts, adding Vitest configuration files, and refactoring several test files to use Vitest's mocking utilities (vi.mock, vi.hoisted). Feedback on these changes highlights a regression where TypeScript type checking was inadvertently disabled in both local and CI test scripts. Additionally, in chat-session.test.ts and template-chat-session.test.ts, the custom stubbing mechanism lacks a corresponding custom restore function to reset the mocked properties, which could lead to test pollution and flaky tests.

Comment thread packages/ai/package.json Outdated
Comment on lines +38 to +39
"test": "run-p --npm-path npm lint test:all",
"test:ci": "node ../../scripts/run_tests_in_ci.js -s test:all",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Removing type-check from the test script and changing test:ci to run test:all instead of test completely disables TypeScript type checking in both local testing and CI. This is a regression that could allow type errors to be merged. Please restore type-check to the test script and configure test:ci to run test.

Suggested change
"test": "run-p --npm-path npm lint test:all",
"test:ci": "node ../../scripts/run_tests_in_ci.js -s test:all",
"test": "run-p --npm-path npm lint type-check test:all",
"test:ci": "node ../../scripts/run_tests_in_ci.js -s test",

Comment thread packages/ai/src/methods/chat-session.test.ts Outdated
Comment thread packages/ai/src/methods/template-chat-session.test.ts Outdated
@Manvi1203
Manvi1203 marked this pull request as draft September 8, 2026 21:37
- Replace anonymous stubs and manual afterEach null-resets with delegated mock implementations in count-tokens and generate-content tests.
- Remove custom restore override in generative-model tests in favor of standard sinon.restore.
- Use native Vitest runIf conditional runners in chrome-adapter and live-session-helpers tests to eliminate ESLint bypass comments.

TAG=agy
CONV=0e4d0bde-5c64-450d-ab5a-2a4f7489262c
controller.abort(abortReason);

await expect(requestPromise).to.be.rejectedWith(
const assertion = expect(requestPromise).to.be.rejectedWith(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Attaching the rejection listener before ticking the clock. Otherwise, ticking the clock triggers the timeout and rejects the promise before the handler is attached, causing an UnhandledPromiseRejection error in Vitest.

mockWebSocket.triggerMessage(new Blob([JSON.stringify({ foo: 2 })]));

await clock.tickAsync(5);
while (received.length < 2) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[test improvement- not related to migration] Blob.text() is an async promise not controlled by fake timers. Wait for both messages to finish decoding before closing the socket.

Comment thread packages/ai/src/websocket.test.ts Outdated
let webSocketStub: SinonStub;

beforeEach(() => {
if (typeof (globalThis as any).WebSocket === 'undefined') {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[test improvement- node tests crashing on node 20] Node 20 lacks global WebSocket (added in Node 22). This dummy class prevents sinon.stub(globalThis, 'WebSocket') from crashing on Node 20.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Our CI runs in Node 22 and the next breaking release will require Node 24 so I think it's safe to not accommodate Node 20 in the tests. This way you also won't have to remember to remove globalThis.WebSocket in the afterEach, which we would have to add here if we kept this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I've removed the fallback.

import sinonChai from 'sinon-chai';
import chaiAsPromised from 'chai-as-promised';
import * as generateContentMethods from './generate-content';

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ESM modules are frozen in Vitest, so sinon.stub() can't modify them directly. This proxy delegates to real code by default, letting us keep existing Sinon stubs.

@Manvi1203
Manvi1203 marked this pull request as ready for review September 9, 2026 22:03
…n-Chai patterns

- Replace legacy Chai assertions with native Vitest matchers
- Eliminate Sinon stub and spy helpers in favor of native vi.spyOn and vi.fn implementations
- Replace custom match and match.any helpers with expect.anything(), expect.objectContaining(), and expect.toSatisfy()
- Establish test/setup.ts to restore mocks and reset timers across test runs
import * as generateContentMethods from './generate-content';
import { expect, vi, type Mock } from 'vitest';

const { mockGenerateContent } = vi.hoisted(() => ({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Had to read the docs a few times to really get it, as this is not super intuitive. I know we have to do this in many places so we don't want to put long comments every time, maybe just something short like "This inserts mockGenerateContent as a spy layer on methods coming from ./generate-content.ts"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a brief comment explaining that this inserts mockGenerateContent as a spy layer on methods coming from ./generate-content.ts.

@@ -437,11 +467,13 @@ describe('ChatSession', () => {
expect((e as unknown as any).name).to.equal('foo');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not totally sure but I think the reason we checked for the error.name instead of error.message was because sinon sets name instead of message. I think in vitest we can just check error.message here and we don't need to add the extra lines to add customErr.name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated! Replaced with a standard new Error('foo') in mockRejectedValue and checked (e as Error).message instead.

Comment thread packages/ai/src/websocket.test.ts Outdated
let webSocketStub: SinonStub;

beforeEach(() => {
if (typeof (globalThis as any).WebSocket === 'undefined') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Our CI runs in Node 22 and the next breaking release will require Node 24 so I think it's safe to not accommodate Node 20 in the tests. This way you also won't have to remember to remove globalThis.WebSocket in the afterEach, which we would have to add here if we kept this.

expect(fetchStub).toHaveBeenCalledTimes(1);
});

it('should throw DOMException if external signal is already aborted', async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change title as we don't check for DOMException anymore because toThrow only accepts one arg I guess?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the test titles to remove DOMException.

export * from './schema';
export * from './googleai';
export {
export type {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This changes the production bundle slightly. I don't think it will hurt anything, but we need to make a changeset to mark the code change, get it released, have a commit to roll back to in case anyone has any errors, etc. Patch is fine.

@Manvi1203 Manvi1203 Sep 15, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done!

@Manvi1203
Manvi1203 requested a review from a team as a code owner September 15, 2026 22:19
@Manvi1203
Manvi1203 requested a review from a team as a code owner September 17, 2026 19:46
# Conflicts:
#	packages/ai/src/googleai-mappers.test.ts
#	packages/ai/src/methods/chat-session.test.ts
#	packages/ai/src/methods/chrome-adapter-browser.test.ts
#	packages/ai/src/methods/generate-content.test.ts
#	packages/ai/src/methods/live-session.test.ts
#	packages/ai/src/methods/template-chat-session.test.ts
#	packages/ai/src/requests/request-helpers.test.ts
#	packages/ai/src/requests/response-helpers.test.ts
@Manvi1203
Manvi1203 merged commit ac44c69 into main Sep 28, 2026
40 checks passed
@Manvi1203
Manvi1203 deleted the feature/vitest-ai branch September 28, 2026 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants