Conversation
|
![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.
| "test:all": "vitest run", | ||
| "test:ci": "node ../../scripts/run_tests_in_ci.js -s test:all", | ||
| "test:browser": "karma start --nocache", | ||
| "test:browser": "vitest run --project=browser", |
There was a problem hiding this comment.
The 'test:integration' script on line 31 still points to Karma ('karma start ./karma.integration.conf.js --nocache'), but the integration test file 'packages/analytics/testing/integration-tests/integration.ts' has been migrated to use Vitest imports ('import { expect, vi } from "vitest"'). Running this script under Karma will now fail because Karma/Webpack cannot resolve or execute Vitest APIs.\n\nPlease update the 'test:integration' script to run via Vitest, or ensure it is covered by 'vitest run'.
| afterEach(async () => { | ||
| await initStub(); | ||
| initStub.restore(); | ||
| await mockInitializeAnalytics(); | ||
| mockInitializeAnalytics.mockReset(); | ||
| _setWrappedGtagFunction(undefined); | ||
| wrappedGtag.mockReset(); | ||
| if (app) { | ||
| return deleteApp(app); | ||
| } | ||
| }); |
There was a problem hiding this comment.
In the 'afterEach' hook, calling 'await mockInitializeAnalytics();' is unnecessary and can be safely removed. This is likely a leftover from the legacy Sinon stub migration where 'await initStub()' was used.
afterEach(async () => {\n mockInitializeAnalytics.mockReset();\n _setWrappedGtagFunction(undefined);\n wrappedGtag.mockReset();\n if (app) {\n return deleteApp(app);\n }\n });| await fetchDynamicConfigWithRetry(app); | ||
| expect(consoleStub.args[0][1]).to.include(fakeMeasurementId); | ||
| consoleStub.restore(); | ||
| expect(consoleStub.mock.calls[0][1]).toContain(fakeMeasurementId); |
There was a problem hiding this comment.
Accessing 'consoleStub.mock.calls[0][1]' directly is fragile because if 'console.warn' is not called, 'calls[0]' will be 'undefined', resulting in a 'TypeError' rather than a descriptive assertion failure. It is better to use 'expect(consoleStub).toHaveBeenCalledWith(...)' for a cleaner and more robust assertion, consistent with other tests in this file.
expect(consoleStub).toHaveBeenCalledWith(\n expect.anything(),\n expect.stringContaining(fakeMeasurementId)\n );| expect(consoleStub.mock.calls[0][1]).toContain(fakeMeasurementId); | ||
| expect(consoleStub.mock.calls[0][1]).toContain('old-measurement-id'); | ||
| expect(consoleStub.mock.calls[0][1]).toContain('does not match'); |
There was a problem hiding this comment.
Accessing 'consoleStub.mock.calls[0][1]' directly is fragile because if 'console.warn' is not called, 'calls[0]' will be 'undefined', resulting in a 'TypeError' rather than a descriptive assertion failure. Adding 'expect(consoleStub).toHaveBeenCalled();' first ensures a clear assertion failure if the warning is not logged.
expect(consoleStub).toHaveBeenCalled();\n const warningMessage = consoleStub.mock.calls[0][1];\n expect(warningMessage).toContain(fakeMeasurementId);\n expect(warningMessage).toContain('old-measurement-id');\n expect(warningMessage).toContain('does not match');| expect(warnStub.mock.calls[0][1]).toContain( | ||
| AnalyticsError.INVALID_ANALYTICS_CONTEXT | ||
| ); | ||
| expect(warnStub.args[0][1]).to.include('Cookies'); | ||
| cookieStub.restore(); | ||
| expect(warnStub.mock.calls[0][1]).toContain('Cookies'); |
There was a problem hiding this comment.
Accessing 'warnStub.mock.calls[0][1]' directly is fragile because if 'console.warn' is not called, 'calls[0]' will be 'undefined', resulting in a 'TypeError' rather than a descriptive assertion failure. Adding 'expect(warnStub).toHaveBeenCalled();' first ensures a clear assertion failure if the warning is not logged.
expect(warnStub).toHaveBeenCalled();\n const warningMessage = warnStub.mock.calls[0][1];\n expect(warningMessage).toContain(\n AnalyticsError.INVALID_ANALYTICS_CONTEXT\n );\n expect(warningMessage).toContain('Cookies');| const warnStub = vi.spyOn(console, 'warn').mockImplementation(() => { | ||
| expect(warnStub.mock.calls[0][1]).toContain('does not match'); | ||
| done(); | ||
| }); |
There was a problem hiding this comment.
Accessing 'warnStub.mock.calls[0][1]' inside the mock implementation itself is fragile. It is cleaner and more robust to use the arguments passed directly to the mock implementation function.
const warnStub = vi.spyOn(console, 'warn').mockImplementation((_tag, message) => {\n expect(message).toContain('does not match');\n done();\n });1 participant
Description
Migrates
@firebase/analyticsunit tests from legacy Karma & Mocha/Chai/Sinon to native Vitest.Testing & Verification