Conversation
🦋 Changeset detectedLatest commit: 329354f 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/auth test suite from Karma, Mocha, Chai, and Sinon to Vitest, which includes deleting the Karma configuration, updating package.json scripts, and refactoring assertions and mocks across numerous test files. Feedback on these changes highlights a critical bug in auth_impl.test.ts where await setTimeout is used incorrectly, causing assertions to be skipped. Additionally, several obsolete test scripts in package.json still reference the deleted Karma runner and need migration or removal, and a conditional test block in redirect.test.ts can be refactored to use Vitest's idiomatic describe.runIf() method.
| expect(auth.currentUser).toBe(null); | ||
| expect(authStateChangedSpy).toHaveBeenCalledTimes(1); | ||
|
|
||
| await setTimeout(() => { |
There was a problem hiding this comment.
Awaiting setTimeout does not pause execution because the global setTimeout returns a timeout ID (or a Timeout object in Node), not a Promise. As a result, this test finishes executing synchronously before the 5-second delay completes, meaning the assertions inside the .then() block are never actually run or verified.\n\nTo fix this, you should use a Promise-based delay or Vitest's fake timers. For example:\n\ntypescript\nawait new Promise(resolve => setTimeout(resolve, 5000));\nawait promiseVar;\nexpect(auth.currentUser).toBe(user);\n
| "test:ci": "node ../../scripts/run_tests_in_ci.js -s test:all", | ||
| "test:integration:local": "run-s --npm-path npm test:node:integration:local test:browser:integration:local test:webdriver", | ||
| "test:browser": "karma start --local", | ||
| "test:browser": "vitest run --project=browser", |
There was a problem hiding this comment.
Since karma.conf.js has been deleted in this PR, several remaining scripts that still reference karma start (such as test:browser:unit, test:browser:integration, test:cordova, etc.) are now broken. Please migrate these remaining test suites to Vitest or remove the obsolete scripts.
| resolver, | ||
| true | ||
| ); |
There was a problem hiding this comment.
Instead of using a ternary operator to conditionally skip the describe block, you can use Vitest's idiomatic describe.runIf() method.
describe.runIf(typeof window !== 'undefined')(
'_getAndClearPendingRedirectStatus',
() => {1 participant
Description
Migrates
@firebase/authunit, browser, and integration tests from legacy Karma & Mocha/Chai/Sinon to Vitest (Node + Browser Chromium).Key Changes
vitest.config.mjsextendingconfig/vitest.base.mjswith dual workspace projects (nodeandbrowserwith Chromium) and removed deprecatedkarma.conf.js.teardownTimeout: 1000and project-specific excludes for platform directories (platform_browser,platform_cordova,platform_react_native,test/integration/**).package.jsontest scripts to Vitest commands (test:all,test:node,test:browser,test:browser:debug).--project=browserand--project=node) to support Auth's dual environments.expect().to.equal,to.be.null,to.be.rejectedWith, etc.) to native Vitest matchers (toBe,toBeNull,rejects.toThrow, etc.) across all 130+ test suites.vi.fn,vi.spyOn,vi.useFakeTimers,vi.advanceTimersByTimeAsync)..catch(() => {})on floating background promises (e.g., inindexed_db.tsandrecaptcha_enterprise_verifier.ts) to eliminate unhandled promise rejections in browser tests.abstract_popup_redirect_operation.ts.export const enum DefaultConfiginsrc/core/auth/auth_impl.tstoexport enum DefaultConfigto preserve enum values at runtime in ESM.src/core/index.tsandsrc/model/public_types.tstoexport type { ... }..changeset/vitest-auth-migration.md(@firebase/auth: patch) documenting the test suite migration, enum preservation, and type re-exports.Testing & Impact
yarn build, API extractor (docgen), andyarn formatsucceed cleanly with 0 unintended API changes.