test: improve coverage for firestore deploy prepare#10356
test: improve coverage for firestore deploy prepare#10356
Conversation
There was a problem hiding this comment.
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.
### Description Mock integration bounds for target selection filters during firestore uploads. ### Scenarios Tested - Loading partial overrides
a29fb74 to
2944815
Compare
|
/gemini review |
There was a problem hiding this comment.
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.
|
/gemini review |
There was a problem hiding this comment.
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.
| projectId: "my-project", | ||
| }); | ||
|
|
||
| it("should create a database with MONGODB_COMPATIBLE when specified on enterprise edition", async () => { |
There was a problem hiding this comment.
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 () => { |
There was a problem hiding this comment.
We might also want to keep this one? I think this covers a scenario where dataAccessMode has a value but edition is null
Description
More AI test coverage.