chore(*): add integration tests #1790
Conversation
… 22 compatibility
… into @invertase/integration-tests
… into @invertase/integration-tests
…ebase/firebase-functions into @invertase/integration-tests
Summary of ChangesHello @dackers86, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a completely revamped integration test suite for Firebase Functions. The previous shell-script-driven, single-function-triggering approach has been replaced with a more robust, TypeScript-based, Vitest-powered framework. This new setup allows for comprehensive testing of both V1 and V2 Firebase Functions across various services like Realtime Database, Firestore, HTTPS, Pub/Sub, Remote Config, Storage, Tasks, Eventarc, Identity, and Scheduler, with a clear orchestration CLI for building, deploying, running, and cleaning up test environments. The changes aim to modernize the testing infrastructure and expand coverage for critical Firebase services. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 is a great and significant refactoring of the integration test suite. The move to Vitest and a dedicated CLI script for orchestration is a major improvement for maintainability.
I've found one critical security issue regarding a hardcoded API key that must be addressed. I've also identified a couple of high-severity bugs in the test runner script (cli.ts) that would prevent it from working as intended, particularly around codebase handling and cleanup.
Additionally, I've left a few medium-severity comments on potential flakiness, a typo, and some inaccuracies in the planning document.
P.S. There's a small typo in the pull request title ('integratino').
| export const config = { | ||
| apiKey: "AIzaSyBBt77mpu6TV0IA2tcNSyf4OltsVu_Z1Zw", | ||
| authDomain: "cf3-integration-tests-v2-qa.firebaseapp.com", | ||
| databaseURL: "https://cf3-integration-tests-v2-qa-default-rtdb.firebaseio.com", | ||
| projectId: "cf3-integration-tests-v2-qa", | ||
| storageBucket: "cf3-integration-tests-v2-qa.firebasestorage.app", | ||
| messagingSenderId: "576826020291", | ||
| appId: "1:576826020291:web:488d568c5d4109df12ed76" | ||
| }; No newline at end of file |
There was a problem hiding this comment.
Hardcoded API keys and project configuration should not be committed to the repository. This is a significant security risk. These values should be loaded from environment variables or a configuration file that is not checked into version control.
| export const config = { | |
| apiKey: "AIzaSyBBt77mpu6TV0IA2tcNSyf4OltsVu_Z1Zw", | |
| authDomain: "cf3-integration-tests-v2-qa.firebaseapp.com", | |
| databaseURL: "https://cf3-integration-tests-v2-qa-default-rtdb.firebaseio.com", | |
| projectId: "cf3-integration-tests-v2-qa", | |
| storageBucket: "cf3-integration-tests-v2-qa.firebasestorage.app", | |
| messagingSenderId: "576826020291", | |
| appId: "1:576826020291:web:488d568c5d4109df12ed76" | |
| }; | |
| export const config = { | |
| apiKey: process.env.FIREBASE_API_KEY, | |
| authDomain: process.env.FIREBASE_AUTH_DOMAIN, | |
| databaseURL: process.env.FIREBASE_DATABASE_URL, | |
| projectId: process.env.GCLOUD_PROJECT, | |
| storageBucket: process.env.FIREBASE_STORAGE_BUCKET, | |
| messagingSenderId: process.env.FIREBASE_MESSAGING_SENDER_ID, | |
| appId: process.env.FIREBASE_APP_ID | |
| }; |
| const firebaseJson = { | ||
| functions: [ | ||
| { | ||
| source: "functions", |
There was a problem hiding this comment.
The writeFirebaseJson function accepts a codebase parameter but doesn't use it when generating firebase.json. This will cause all functions to be deployed to the default codebase, and the cleanup logic, which seems to rely on a unique codebase per run, will likely fail. The codebase should be added to the function's definition in firebase.json.
| source: "functions", | |
| source: "functions", | |
| codebase: codebase, |
integration_test/cli.ts
Outdated
| async function cleanupFunctions(codebase: string): Promise<void> { | ||
| console.log(`Cleaning up functions with RUN_ID: ${runId}...`); | ||
| await execCommand("firebase", ["functions:delete", runId, "--force"], {}, integrationTestDir); | ||
| console.log("Functions cleaned up successfully"); | ||
| } |
There was a problem hiding this comment.
The cleanupFunctions function incorrectly uses the global runId constant instead of its codebase parameter for logging and in the firebase functions:delete command. This should be corrected to use the codebase parameter to ensure the correct set of functions is deleted.
| async function cleanupFunctions(codebase: string): Promise<void> { | |
| console.log(`Cleaning up functions with RUN_ID: ${runId}...`); | |
| await execCommand("firebase", ["functions:delete", runId, "--force"], {}, integrationTestDir); | |
| console.log("Functions cleaned up successfully"); | |
| } | |
| async function cleanupFunctions(codebase: string): Promise<void> { | |
| console.log(`Cleaning up functions with RUN_ID: ${codebase}...`); | |
| await execCommand("firebase", ["functions:delete", codebase, "--force"], {}, integrationTestDir); | |
| console.log("Functions cleaned up successfully"); | |
| } |
| await writeEnvFile(runId); | ||
| await deployFunctions(runId); | ||
| console.log("Waiting 20 seconds for deployments fully provision before running tests..."); | ||
| await new Promise((resolve) => setTimeout(resolve, 20_000)); |
There was a problem hiding this comment.
Using a hardcoded setTimeout to wait for deployments can lead to flaky tests. If the deployment takes longer than 20 seconds, the tests will fail. A more robust approach would be to poll for the status of the functions until they are ready. For an internal integration test script, this might be an acceptable trade-off to avoid added complexity, but it's worth considering improving this if tests become flaky.
integration_test/PLAN.md
Outdated
| exports.firestoreOnCreate = onDocumentCreate((event) => { | ||
| firestore.collection(...).doc(...).set(event)'; | ||
| }) | ||
| ``` | ||
|
|
||
| ```ts | ||
| exports.firestoreOnCreate = onDocumentCreate((event) => { | ||
| await sendEvent(event); | ||
| }) | ||
|
|
||
| let topic; | ||
|
|
||
| async function(name: string, event: any): Promise<void> { | ||
| topic ??= await pubsub.createTopic(process.env.RUN_ID); | ||
| await topic.publishMessage({ name, event }) | ||
| } | ||
| ``` | ||
|
|
||
| # Test Files | ||
|
|
||
| ``` | ||
| describe('triggers the correct document event', () => { | ||
| beforeAll(() => { | ||
| let event = await new Promise((resolve) => { | ||
| setUpEventListener('firestoreOnCreate', (event) => { | ||
| resolve(event); | ||
| }); | ||
|
|
||
| await admin().firestore().collection(process.env.RUN_ID).doc('foo'); | ||
| }); | ||
|
|
||
| }); | ||
|
|
||
| test('whatever...'); | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
The code snippets in this plan contain a few syntax errors that could be misleading:
- On line 22, there's a stray single quote:
firestore.collection(...).doc(...).set(event)'; - On line 33,
async function(name: string, event: any): Promise<void> {is not a valid function declaration as it's missing a name. - On line 49,
awaitis used inside anew Promiseexecutor that is notasync, which is a syntax error.
While this is a planning document, correcting these could prevent confusion.
| }, | ||
| }) | ||
| .onDispatch(async (data, event) => { | ||
| await sendEvent("onTaskDispatchedV1 ", { |
There was a problem hiding this comment.
There appears to be a typo in the event name string passed to sendEvent. The string "onTaskDispatchedV1 " has a trailing space, which will likely cause the test waiting for the "onTaskDispatchedV1" event to time out.
| await sendEvent("onTaskDispatchedV1 ", { | |
| await sendEvent("onTaskDispatchedV1", { |
dackers86
force-pushed
the
@invertase/integration-tests
branch
from
1de7c93 to
12a54d7
Compare
dackers86
changed the title
6 participants
No description provided.