Skip to content

test: improve coverage for firestore deploy prepare#10356

Open
joehan wants to merge 5 commits intonextfrom
test/firestore-prepare
Open

test: improve coverage for firestore deploy prepare#10356
joehan wants to merge 5 commits intonextfrom
test/firestore-prepare

Conversation

@joehan
Copy link
Copy Markdown
Member

@joehan joehan commented Apr 15, 2026

Description

More AI test coverage.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the unit tests for Firestore deployment preparation, introducing new tests for filtering flags and invalid edition handling. However, the changes result in a significant loss of test coverage for database creation logic and validation scenarios that were previously covered. Feedback also highlights a violation of the style guide regarding the use of 'any' types and suggests improving assertions to verify stub parameters for better reliability.

Comment thread src/deploy/firestore/prepare.spec.ts
Comment thread src/deploy/firestore/prepare.spec.ts Outdated
Comment thread src/deploy/firestore/prepare.spec.ts
### Description
Mock integration bounds for target selection filters during firestore uploads.

### Scenarios Tested
- Loading partial overrides
@joehan joehan force-pushed the test/firestore-prepare branch from a29fb74 to 2944815 Compare April 20, 2026 12:35
@joehan
Copy link
Copy Markdown
Member Author

joehan commented Apr 22, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the test suite for Firestore preparation, simplifying the setup and adding new test cases for filtering flags and error handling. However, the refactor significantly reduced test coverage by removing detailed assertions for database creation parameters like data access modes and editions. Additionally, the new tests frequently use any and unknown as type escape hatches, which violates the repository's TypeScript style guide. Feedback includes restoring the missing coverage and adopting type-safe mocking practices.

Comment thread src/deploy/firestore/prepare.spec.ts
Comment thread src/deploy/firestore/prepare.spec.ts Outdated
Comment thread src/deploy/firestore/prepare.spec.ts Outdated
@joehan
Copy link
Copy Markdown
Member Author

joehan commented Apr 22, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the Firestore preparation tests by introducing a TestContext interface and updating the Sinon mocking approach. The review identifies a missing import for DeployOptions that causes a compilation error and highlights a regression in test coverage due to the removal of detailed database configuration tests. These issues should be addressed to maintain code quality and functional verification.

Comment thread src/deploy/firestore/prepare.spec.ts
Comment thread src/deploy/firestore/prepare.spec.ts
@joehan joehan requested a review from aalej April 23, 2026 22:27
@joehan joehan changed the base branch from main to next April 23, 2026 22:37
projectId: "my-project",
});

it("should create a database with MONGODB_COMPATIBLE when specified on enterprise edition", async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to keep this test?

expect(compileStub).to.have.been.calledOnce;
});

it("should throw an error when dataAccessMode is specified without edition (defaults to standard)", async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might also want to keep this one? I think this covers a scenario where dataAccessMode has a value but edition is null

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants