Conversation
🦋 Changeset detectedLatest commit: 69adcca The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the @firebase/analytics test suite from Karma, Mocha, Chai, and Sinon to Vitest, updating package scripts, assertions, and mocking utilities across multiple test files. The review feedback points out that the test:integration script in package.json still references Karma despite the integration tests being migrated, and identifies a leftover asynchronous mock call in api.test.ts. Additionally, several test assertions directly accessing mock.calls are flagged as fragile and should be refactored to use safer Vitest matchers like toHaveBeenCalledWith or explicit call checks.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the @firebase/analytics test suite from Karma, Mocha, Chai, and Sinon to Vitest, updating configuration files, package scripts, and test assertions accordingly. The review feedback highlights a few important issues: first, the simplified test:all script in package.json skips integration tests, which should be restored; second, the app variable in api.test.ts should be reset to undefined in afterEach to prevent state leakage; and third, the console.warn mock in the integration tests should be made more robust by filtering for the expected warning message rather than asserting on the first warning received.
| "dev": "rollup -c -w", | ||
| "test": "run-p --npm-path npm lint test:all", | ||
| "test:all": "run-p --npm-path npm test:browser test:integration", | ||
| "test:all": "vitest run", |
There was a problem hiding this comment.
The test:all script has been simplified to vitest run, which only runs the default unit tests. This means integration tests (configured in vitest.integration.config.mjs) are no longer executed as part of test:all or in CI (via test:ci). Please restore the execution of both unit and integration tests.
| "test:all": "vitest run", | |
| "test:all": "run-p --npm-path npm test:browser test:integration", |
| if (app) { | ||
| return deleteApp(app); | ||
| } |
There was a problem hiding this comment.
The suite-level app variable is deleted in afterEach but not reset to undefined. Resetting it to undefined prevents potential state leakage or double-deletion attempts in subsequent tests.
if (app) {
await deleteApp(app);
app = undefined as any;
}| vi.spyOn(console, 'warn').mockImplementation((_tag, message) => { | ||
| expect(message).toContain('does not match'); | ||
| resolve(); | ||
| }); |
There was a problem hiding this comment.
The mock implementation of console.warn asserts that the warning message contains 'does not match' on the very first warning received. If any other warning (such as a browser context or cookie warning) is logged first, the assertion will fail and the test will fail prematurely. Filtering the arguments to find the expected warning message makes the test much more robust.
vi.spyOn(console, 'warn').mockImplementation((...args) => {
const hasMatch = args.some(arg => typeof arg === 'string' && arg.includes('does not match'));
if (hasMatch) {
resolve();
}
});1 participant
Summary
Migrates
@firebase/analyticsunit and integration tests from legacy Karma (Mocha / Chai / Sinon) to Vitest (Browser Chromium via Playwright).Description
karma.conf.jsandkarma.integration.conf.jswithvitest.config.mjsandvitest.integration.config.mjsextendingconfig/vitest.base.mjs. Configured browser runner targeting Chromium via Playwright with defensive project filtering (if (config.test?.projects)). Addedtest/types/vitest-globals.d.tsand configuredtest/setup.tswith global console/logger stubs to keep test logs clean.@firebase/analyticsrelies on browser APIs (window,dataLayer,gtag,IndexedDB, DOM script injection). Both unit tests and integration tests run in thebrowserproject via@vitest/browser-playwright..to.equal,.to.deep.equal,.to.be.true,.to.throw) to native Vitest matchers (.toBe(),.toEqual(),.toBe(true),.toThrow()). Converted Mocha hooks (after,afterEach) to Vitest equivalents (afterAll,afterEach).vi.fn(),vi.spyOn(),vi.mock()). Replaced fragile array index mock assertions with robust matcher assertions (toHaveBeenLastCalledWith(...)). Added internal_setWrappedGtagFunctionhelper to cleanly stubgtagcalls in ESM environments.testing/integration-tests/integration.tsfrom Mocha'sdonecallback to Promise-based execution. Excludedtesting/**fromrollup.config.jsto ensure clean library builds in CI environments.package.jsonscripts (test,test:all,test:browser,test:integration,test:ci,test:browser:debug) to use Vitest and cleaned up legacy Karma configs.Performance