Conversation
|
There was a problem hiding this comment.
Code Review
This pull request migrates the Firestore test suite from Karma and Mocha/Chai to Vitest, updating package scripts, test runners, and refactoring assertions across numerous test files. The reviewer identified several critical issues with the migration: running tests in debug mode with --inspect-brk directly on Vitest will fail; the platform check in base64.ts incorrectly excludes 'browser_lite'; vi should be explicitly imported in test/setup.ts for robustness; and multiple test files incorrectly use Vitest's toBeCloseTo with an absolute delta instead of a decimal precision argument, leading to either overly loose or overly strict assertions.
b2918c3 to
854fd21
Compare
854fd21 to
d5adfb2
Compare
…all webkit macos bindings
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the Firestore integration and unit tests from Karma, Mocha, and Chai to Vitest. This includes updating package scripts, replacing Chai assertions with Vitest's native matchers, and restructuring setup files. The review feedback highlights several opportunities to write more idiomatic Vitest assertions by replacing manual absolute difference calculations with the toBeCloseTo matcher, as well as removing a redundant registerFirestoreLite() call in the test setup.
Summary
Migrates
@firebase/firestore(packages/firestore) andintegration/firestoreunit, integration, and lite test suites (test/unit/**,test/integration/**, andtest/lite/**) from Karma + Mocha + Chai + Sinon to Vitest across Node and Browser (Playwright Chromium, Firefox, and WebKit) environments.Description
Test Runner Config, Setup & CI Workflows
packages/firestore/vitest.config.mjskarma.conf.jsandmocha. Extends sharedconfig/vitest.base.mjswithnodeandbrowserVitest projects, mapssrc/platform/*aliases (node,browser,node_lite,browser_lite), and injects build-time env flags (__FIRESTORE_TARGET_BACKEND__,__FIRESTORE_USE_MOCK_PERSISTENCE__) via Vite'sdefinefor browser tests.integration/firestore/vitest.config.mjsintegration/firestorepackage (tested under bothmemoryandpersistencemodes).packages/firestore/test/setup.tsbootstrap.tsfiles (test/unit/bootstrap.ts,test/integration/bootstrap.ts,test/lite/bootstrap.ts). Registers both full Firestore (registerFirestore) and Lite (registerFirestoreLite) per Vitest worker, polyfillsBufferin the browser, and restores timers/mocks (vi.useRealTimers(),vi.restoreAllMocks()) after each test.packages/firestore/scripts/run-tests.tsvitestCLI instead ofnyc/mochawhile preserving existing CLI flags (--platform,--persistence,--emulator,--prod).packages/firestore/package.json&integration/firestore/package.jsontest:*scripts (test:node,test:browser,test:firefox,test:webkit,test:lite:*) to use Vitest and Playwright browser providers..github/workflows/test-changed-firestore.yml&.github/workflows/test-changed-firestore-integration.ymlchromium,firefox,webkit) in CI where Karma browser launchers were previously used.Firestore Test Harness & Custom Helpers
packages/firestore/test/util/mocha_extensions.tsit.skipEnterprise,it.skipEmulator,describe.skipClassic, etc.). Why it changed: In Mocha,it.skipanddescribe.skipare static function objects that can be mutated once at import time. In Vitest,.skipis a property getter that returns a newly wrapped function instance on every access. We added agetSkip()helper so chaining (e.g.it.skipEnterprise.skipEmulator(...)) re-attaches the custom modifiers onto newly returned.skipfunctions.packages/firestore/test/util/equality_matcher.tstoEqualplugin override with Vitest'sexpect.addEqualityTesters(...)so Firestore types with.isEqual()(or customEqualsFnmatchers) work seamlessly insideexpect(a).toEqual(b)and nested arrays/objects.packages/firestore/test/util/helpers.ts&packages/firestore/test/integration/util/helpers.tsthis.test.fullTitle()withexpect.getState().currentTestNameand ensuresapiDescribeproperly skips suites at definition time when persistence/backend flags disable them.packages/firestore/test/integration/prime_backend.test.tsbefore()hook with zeroit()blocks. Vitest fails if a.test.tsfile has no test suite (Error: No test suite found in file). WrappedbeforeAllinsidedescribe('Prime Backend', () => { ... it('primes backend', () => {}); }).Mechanical Test Suite Migrations (~154
*.test.tsfiles )All remaining files under
packages/firestore/test/{unit,integration,lite}/**andintegration/firestore/test/**are mechanical translations following the cheat sheet below.Vitest Equivalents
Vitest uses a Jest-compatible
expectand mocking API (vi) while keepingdescribe/ittest structure:before(() => ...)/after(() => ...)beforeAll(() => ...)/afterAll(() => ...)(beforeEach/afterEachare unchanged)===)expect(x).to.equal(y)/expect(x).to.be.trueexpect(x).toBe(y)/expect(x).toBe(true)expect(x).to.deep.equal(y)expect(x).toEqual(y)expect(x).to.be.closeTo(expected, delta)expect(x).toBeCloseTo(expected, precisionDigits)expect(arr).to.have.members([a, b])expect(arr).toEqual(expect.arrayContaining([a, b]))await expect(promise).to.eventually.be.rejectedWith(/msg/)await expect(promise).rejects.toThrow(/msg/)done)it('test', done => { ... done(); })it('test', () => new Promise<void>(resolve => { ... resolve(); }))(Vitest deprecates callback-styledonein favor of Promises)sinon.spy(),sinon.stub(obj, 'm'),sinon.useFakeTimers()vi.fn(),vi.spyOn(obj, 'm'),vi.useFakeTimers()this.test.fullTitle()(requiresfunction()binding)expect.getState().currentTestName(works in arrow functions)Other Changes
onUnhandledErrorinvitest.config.mjs:In
database.test.ts, the 4onSnapshotResumemalformed-bundle tests (bundle: 'BadData') trigger a synchronousBundleReaderImplconstructor rejection (Invalid bundle format: Reached the end of bundle when a length string is expected.) beforegetSyncEngine()finishes opening IndexedDB on theasyncQueueand attaches a.catchhandler toreader.getMetadata(). Rather than disabling unhandled error tracking globally,config.test.onUnhandledErrorreturnsfalseonly for that specificInvalid bundle formaterror so any genuine unhandled rejection in unit, lite, or integration tests still fails the test run.isolate: false, fileParallelism: false):In the
browserproject, tests run in a shared Playwright browser context rather than parallel isolated iframes. This matches Karma's single-tab execution model and avoids cross-iframe IndexedDB lock contention (SimpleDb.delete) during persistence tests.describe.skip):When a test file is conditionally disabled (e.g.,
large_document.test.tswhenRUN_LARGE_DOC_TESTSis unset, or pipeline emulator tests when running against prod), Vitest requires the suite to be marked with.skip(apiDescribe.skip(...)) rather than returning early insidedescribe(...)with 0 registered tests (which Vitest flags as an empty suite error).Performance Improvement
test:node)ts-node)test:browser:unit)