Modernize Test Tooling in uppercase-rtdb (Mocha to Pure Jest)#1292
Modernize Test Tooling in uppercase-rtdb (Mocha to Pure Jest)#1292inlined wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates dependencies and the Node engine version, and migrates the testing framework from Mocha/Chai/Sinon to Jest. The review feedback highlights two critical issues in the new Jest tests: first, attempting to directly reassign the read-only admin.database getter in the offline tests will fail, and should instead be managed using Jest's spy utilities; second, the database cleanup in the online tests is asynchronous and must be awaited before calling test.cleanup() to prevent race conditions and uninitialized app errors.
| let oldDatabase; | ||
| beforeAll(() => { | ||
| oldDatabase = admin.database; | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| admin.database = oldDatabase; | ||
| }); | ||
|
|
||
| it('should return a 303 redirect', (done) => { | ||
| const refParam = '/messages'; | ||
| const pushParam = { original: 'input' }; | ||
|
|
||
| const pushStub = jest.fn().mockImplementation((val) => val.original === pushParam.original ? Promise.resolve({ ref: 'new_ref' }) : undefined); | ||
| const refStub = jest.fn().mockImplementation((path) => path === refParam ? { push: pushStub } : undefined); | ||
| const databaseStub = jest.fn().mockReturnValue({ ref: refStub }); | ||
|
|
||
| jest.spyOn(admin, 'database', 'get').mockReturnValue(databaseStub); |
There was a problem hiding this comment.
Attempting to reassign admin.database directly (e.g., admin.database = oldDatabase) will fail or throw a TypeError in strict mode because database is a read-only getter property on the admin object. Since you are using Jest, you should leverage the spy returned by jest.spyOn and call mockRestore() on it in afterAll to properly clean up the mock without manual reassignment.
| let oldDatabase; | |
| beforeAll(() => { | |
| oldDatabase = admin.database; | |
| }); | |
| afterAll(() => { | |
| admin.database = oldDatabase; | |
| }); | |
| it('should return a 303 redirect', (done) => { | |
| const refParam = '/messages'; | |
| const pushParam = { original: 'input' }; | |
| const pushStub = jest.fn().mockImplementation((val) => val.original === pushParam.original ? Promise.resolve({ ref: 'new_ref' }) : undefined); | |
| const refStub = jest.fn().mockImplementation((path) => path === refParam ? { push: pushStub } : undefined); | |
| const databaseStub = jest.fn().mockReturnValue({ ref: refStub }); | |
| jest.spyOn(admin, 'database', 'get').mockReturnValue(databaseStub); | |
| let databaseSpy; | |
| afterAll(() => { | |
| if (databaseSpy) { | |
| databaseSpy.mockRestore(); | |
| } | |
| }); | |
| it('should return a 303 redirect', (done) => { | |
| const refParam = '/messages'; | |
| const pushParam = { original: 'input' }; | |
| const pushStub = jest.fn().mockImplementation((val) => val.original === pushParam.original ? Promise.resolve({ ref: 'new_ref' }) : undefined); | |
| const refStub = jest.fn().mockImplementation((path) => path === refParam ? { push: pushStub } : undefined); | |
| const databaseStub = jest.fn().mockReturnValue({ ref: refStub }); | |
| databaseSpy = jest.spyOn(admin, 'database', 'get').mockReturnValue(databaseStub); |
| afterAll(() => { | ||
| test.cleanup(); | ||
| admin.database().ref('messages').remove(); | ||
| }); |
There was a problem hiding this comment.
The database cleanup operation admin.database().ref('messages').remove() returns a Promise and is asynchronous. Without awaiting it, the cleanup might not complete before the test process exits, potentially leading to race conditions or test pollution. Additionally, calling test.cleanup() before the database operation is completed can uninitialize the Firebase app, causing the database call to fail. We should make the hook async and await the database removal before calling test.cleanup().
| afterAll(() => { | |
| test.cleanup(); | |
| admin.database().ref('messages').remove(); | |
| }); | |
| afterAll(async () => { | |
| await admin.database().ref('messages').remove(); | |
| test.cleanup(); | |
| }); |
1. Original CVEs
GHSA-23c5-xmqv-rm74, GHSA-25h7-pfq9-p65f, GHSA-2pr8-phx7-x9h3, GHSA-37ch-88jc-xwx2, GHSA-3ppc-4f35-3m26, GHSA-5375-pq7m-f5r2, GHSA-66ff-xgx4-vchm, GHSA-685m-2w69-288q, GHSA-6rw7-vpxm-498p, GHSA-73rr-hh4g-fpgx, GHSA-75px-5xx7-5xc7, GHSA-7r86-cg39-jmmj, GHSA-99f4-grh7-6pcq, GHSA-f8q6-p94x-37v3, GHSA-fx83-v9x8-x52w, GHSA-g78m-2chm-r7qv, GHSA-gxpj-cx7g-858c, GHSA-jggg-4jg4-v7c6, GHSA-jvwf-75h9-cwgg, GHSA-mh29-5h37-fv8m, GHSA-q6x5-8v7m-xcrf, GHSA-q8mj-m7cp-5q26, GHSA-rf6f-7fwh-wjgh, GHSA-w7fw-mjwx-w883, GHSA-xq3m-2v4x-88gg2. CVEs Fixed
24 vulnerabilities removed. Notably eliminated
GHSA-73rr-hh4g-fpgx(diff, pulled in via Mocha) and underlying legacy Mocha parsing bugs.3. CVEs Introduced
None
4. CVEs Remaining
GHSA-w5hq-g745-h8pqRemaining Vulnerable Transitive Dependencies:
uuid-> Base package:firebase-admin(via@google-cloud/storage->gaxios/teeny-request)5. Changes Made
mocha,chai,chai-as-promised, andsinondevelopment packages.jest(^29.7.0).test.offline.jsandtest.online.jsinto pureJesttest suites (*.test.js), replacing allSinonstubs/spies with nativejest.fn()andjest.spyOn()equivalents.6. Automated Test Strategy
Executed comprehensive automated unit and online integration test suites (
npm test) running inside the official Firebase Local Emulator Suite (Runtime and Realtime Database emulators), validating Cloud Function triggers, HTTP responses, and database transactions.