feat(firestore): initial support#245
Conversation
![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 introduces the google_firestore_v1 package, including Terraform configurations for Firestore resources, Firebase emulator setup, and initial library generation with tests and examples. Several issues were identified regarding incorrect package naming in imports within the example and test files, which would lead to compilation errors. Additionally, the librarian.yaml configuration contains a filename mismatch in the 'keep' list that would cause the newly added test file to be deleted during regeneration, and a comment in the example code incorrectly references the Generative Language API instead of Firestore.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the Google Cloud Firestore v1 library, including Terraform configurations for provisioning Firestore resources, Firebase emulator setup for local development, and the generated Dart client with accompanying tests and examples. The review feedback suggests improving the Terraform configuration by using variables for location instead of hardcoded values. Additionally, for the test suite, it is recommended to use environment variables for the emulator endpoint, remove an unnecessary async keyword from the test registration function, and include the required import for environment access.
| resource "google_firestore_database" "default" { | ||
| project = var.project | ||
| name = "(default)" | ||
| location_id = "us-central1" |
There was a problem hiding this comment.
The location_id is hardcoded to us-central1. Since a region variable is already defined and used for other resources in this file, it should be used here as well to ensure consistency and allow for easier multi-region deployments.
location_id = var.region
|
|
||
| @TestOn('vm') | ||
| library; | ||
|
|
There was a problem hiding this comment.
Add import 'dart:io'; to allow accessing environment variables via Platform.environment.
import 'dart:io';
|
|
||
| const databaseId = '(default)'; | ||
|
|
||
| void firestoreTest(Future<Firestore> Function() createFirestore) async { |
There was a problem hiding this comment.
The firestoreTest function is used to register tests and setup/teardown blocks. It should not be marked as async because it does not perform any asynchronous operations itself. In Dart's package:test, registering tests inside an async function is generally discouraged as it can lead to race conditions or tests not being discovered if an await is introduced before the test registration calls.
void firestoreTest(Future<Firestore> Function() createFirestore) {| firestoreTest( | ||
| () async => Firestore( | ||
| client: http.Client(), | ||
| endPoint: Uri.http('127.0.0.1:8080'), |
There was a problem hiding this comment.
The emulator endpoint is hardcoded to 127.0.0.1:8080. It should instead respect the FIRESTORE_EMULATOR_HOST environment variable if it is set, matching the instructions provided in the README.md and allowing for more flexible test environments.
endPoint: Uri.http(Platform.environment['FIRESTORE_EMULATOR_HOST'] ?? '127.0.0.1:8080'),
2 participants
Adds the configuration, tests, and example for a new generated package:
package:google_cloud_firestore_v1Adds terraform configuration for firestore for integration tests.
Before this change, only
package:google_cloud_storagehad tests that used the firebase emulator suite. The new firestore package adds additional emulator tests so:google_cloud_storage/emulator_testto a top-levelfirestore_emulatorsdirectory..github/workflows/gcs_emulator_tests.yamltodart_checks.yamland is run against all tests (with the"firebase_emulator"tag) instead of just cloud storage tests.