-
Notifications
You must be signed in to change notification settings - Fork 213
@invertase/rebased v2 it robust #1719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @cabljac, 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 represents a major upgrade to the Firebase Functions integration test suite. It transitions from a bespoke testing approach to a more modern, robust, and extensible Jest-based system. The changes aim to improve the reliability of the tests, expand coverage for both v1 and v2 function triggers across various Firebase services, and facilitate easier integration into CI/CD pipelines.
Highlights
- Test Suite Overhaul: The entire integration test suite has been significantly refactored and migrated from a custom testing framework to Jest, enhancing test structure and maintainability.
- Robust Deployment & Cleanup: A new orchestration system (
run.ts
,deployment-utils.ts
) has been introduced to manage Firebase Functions deployment and cleanup, incorporating rate limiting and retry logic for increased reliability. - Expanded v1 & v2 Coverage: Comprehensive test coverage has been added or refactored for both Firebase Functions v1 and v2 across various services, including Auth, Realtime Database, Firestore, Pub/Sub, Remote Config, Storage, Tasks, Test Lab, Alerts, Eventarc, Identity, and Scheduler.
- CI/CD Integration: A
cloudbuild.yaml
file has been added to enable continuous integration for the new integration test suite, streamlining automated testing. - Environment Configuration: Standardized environment variable loading and configuration (
.env.example
,utils.ts
) have been implemented for easier setup and execution of tests.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
||
it("should have a correct ref url", () => { | ||
expect(loggedContext?.url).toMatch( | ||
new RegExp(`^https://${projectId}(-default-rtdb)*.firebaseio.com/dbTests/${testId}/start$`) |
Check failure
Code scanning / CodeQL
Regular expression injection High test
environment variable
This regular expression is constructed from a
environment variable
it("should have refs resources", () => { | ||
expect(loggedContext?.resource.name).toMatch( | ||
new RegExp( | ||
`^projects/_/instances/${projectId}(-default-rtdb)*/refs/dbTests/${testId}/start` |
Check failure
Code scanning / CodeQL
Regular expression injection High test
environment variable
This regular expression is constructed from a
environment variable
|
||
it("should have a correct ref url", () => { | ||
expect(loggedContext?.url).toMatch( | ||
new RegExp(`^https://${projectId}(-default-rtdb)*.firebaseio.com/dbTests/${testId}/start$`) |
Check failure
Code scanning / CodeQL
Regular expression injection High test
environment variable
This regular expression is constructed from a
environment variable
it("should have refs resources", () => { | ||
expect(loggedContext?.resource.name).toMatch( | ||
new RegExp( | ||
`^projects/_/instances/${projectId}(-default-rtdb)*/refs/dbTests/${testId}/start$` |
Check failure
Code scanning / CodeQL
Regular expression injection High test
environment variable
This regular expression is constructed from a
environment variable
|
||
it("should have a correct ref url", () => { | ||
expect(loggedContext?.url).toMatch( | ||
new RegExp(`^https://${projectId}(-default-rtdb)*.firebaseio.com/dbTests/${testId}/start$`) |
Check failure
Code scanning / CodeQL
Regular expression injection High test
environment variable
This regular expression is constructed from a
environment variable
it("should have refs resources", () => { | ||
expect(loggedContext?.resource.name).toMatch( | ||
new RegExp( | ||
`^projects/_/instances/${projectId}(-default-rtdb)*/refs/dbTests/${testId}/start$` |
Check failure
Code scanning / CodeQL
Regular expression injection High test
environment variable
This regular expression is constructed from a
environment variable
|
||
it("should have a correct ref url", () => { | ||
expect(loggedContext?.url).toMatch( | ||
new RegExp(`^https://${projectId}(-default-rtdb)*.firebaseio.com/dbTests/${testId}/start$`) |
Check failure
Code scanning / CodeQL
Regular expression injection High test
environment variable
This regular expression is constructed from a
environment variable
it("should have refs resources", () => { | ||
expect(loggedContext?.resource.name).toMatch( | ||
new RegExp( | ||
`^projects/_/instances/${projectId}(-default-rtdb)*/refs/dbTests/${testId}/start$` |
Check failure
Code scanning / CodeQL
Regular expression injection High test
environment variable
This regular expression is constructed from a
environment variable
b060a63
to
fa98a7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This is a significant and well-executed overhaul of the integration test suite. The migration to a modern testing framework with Jest, a robust test runner, and a clear, modular structure for tests is a huge improvement. The addition of comprehensive tests for both v1 and v2 functions greatly increases coverage.
I've identified one critical issue in the deployment logic that causes all functions to be redeployed multiple times, which is likely the source of the flakiness you've observed. I've also provided several suggestions to improve type safety and code robustness. Addressing these points will make this excellent contribution even better.
// Deploy functions in batches | ||
const batches = []; | ||
for (let i = 0; i < functionsToDeploy.length; i += BATCH_SIZE) { | ||
batches.push(functionsToDeploy.slice(i, i + BATCH_SIZE)); | ||
} | ||
|
||
for (let i = 0; i < batches.length; i++) { | ||
const batch = batches[i]; | ||
console.log(`Deploying batch ${i + 1}/${batches.length} (${batch.length} functions)`); | ||
|
||
try { | ||
await pRetry( | ||
async () => { | ||
await deploymentLimiter(async () => { | ||
await client.deploy({ | ||
only: "functions", | ||
force: true, | ||
}); | ||
}); | ||
}, | ||
{ | ||
retries: MAX_RETRIES, | ||
onFailedAttempt: (error: any) => { | ||
console.log( | ||
`❌ Deployment failed (attempt ${error.attemptNumber}/${MAX_RETRIES + 1}):`, | ||
error.message | ||
); | ||
// Log detailed error information during retries | ||
if (error.children && error.children.length > 0) { | ||
console.log("📋 Detailed deployment errors:"); | ||
error.children.forEach((child: any, index: number) => { | ||
console.log(` ${index + 1}. ${child.message || child}`); | ||
if (child.original) { | ||
console.log( | ||
` Original error message: ${child.original.message || "No message"}` | ||
); | ||
console.log(` Original error code: ${child.original.code || "No code"}`); | ||
console.log( | ||
` Original error status: ${child.original.status || "No status"}` | ||
); | ||
} | ||
}); | ||
} | ||
// Log the full error structure for debugging | ||
console.log("🔍 Error details:"); | ||
console.log(` - Message: ${error.message}`); | ||
console.log(` - Status: ${error.status}`); | ||
console.log(` - Exit code: ${error.exit}`); | ||
console.log(` - Attempt: ${error.attemptNumber}`); | ||
console.log(` - Retries left: ${error.retriesLeft}`); | ||
}, | ||
} | ||
); | ||
|
||
console.log(`✅ Batch ${i + 1} deployed successfully`); | ||
|
||
// Add delay between batches | ||
if (i < batches.length - 1) { | ||
console.log(`Waiting ${DELAY_BETWEEN_BATCHES}ms before next batch...`); | ||
await sleep(DELAY_BETWEEN_BATCHES); | ||
} | ||
} catch (error: any) { | ||
console.error(`❌ Failed to deploy batch ${i + 1}:`, error); | ||
// Log detailed error information | ||
if (error.children && error.children.length > 0) { | ||
console.log("📋 Detailed deployment errors:"); | ||
error.children.forEach((child: any, index: number) => { | ||
console.log(` ${index + 1}. ${child.message || child}`); | ||
if (child.original) { | ||
console.log(` Original error message: ${child.original.message || "No message"}`); | ||
console.log(` Original error code: ${child.original.code || "No code"}`); | ||
console.log(` Original error status: ${child.original.status || "No status"}`); | ||
} | ||
}); | ||
} | ||
// Log the full error structure for debugging | ||
console.log("🔍 Error details:"); | ||
console.log(` - Message: ${error.message}`); | ||
console.log(` - Status: ${error.status}`); | ||
console.log(` - Exit code: ${error.exit}`); | ||
console.log(` - Attempt: ${error.attemptNumber}`); | ||
console.log(` - Retries left: ${error.retriesLeft}`); | ||
throw error; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deployment logic here seems to have a flaw. The code attempts to deploy functions in batches, but the client.deploy({ only: "functions", force: true })
call inside the loop will attempt to deploy all functions in the codebase on every iteration. This means if you have 3 batches, you are deploying all functions 3 times, which is inefficient and likely the cause of the deployment flakiness mentioned in the README.
Since functions.yaml
is used, you typically deploy the entire codebase at once, not individual functions. The batching logic for deployment is therefore not effective here.
I suggest removing the batching loop for deployment and just attempting to deploy all functions once with retries. The functionsToDeploy
parameter is also unused and could be removed for clarity, though I've left it in the suggestion to minimize changes.
// The batching logic here is incorrect for manifest-based deploys.
// It will attempt to deploy all functions multiple times.
// A single deploy call with retries is what's needed.
try {
await pRetry(
async () => {
await deploymentLimiter(async () => {
await client.deploy({
only: "functions",
force: true,
});
});
},
{
retries: MAX_RETRIES,
onFailedAttempt: (error: any) => {
console.log(
`❌ Deployment failed (attempt ${error.attemptNumber}/${MAX_RETRIES + 1}):`,
error.message
);
// Log detailed error information during retries
if (error.children && error.children.length > 0) {
console.log("📋 Detailed deployment errors:");
error.children.forEach((child: any, index: number) => {
console.log(` ${index + 1}. ${child.message || child}`);
if (child.original) {
console.log(
` Original error message: ${child.original.message || "No message"}`
);
console.log(` Original error code: ${child.original.code || "No code"}`);
console.log(
` Original error status: ${child.original.status || "No status"}`
);
}
});
}
// Log the full error structure for debugging
console.log("🔍 Error details:");
console.log(` - Message: ${error.message}`);
console.log(` - Status: ${error.status}`);
console.log(` - Exit code: ${error.exit}`);
console.log(` - Attempt: ${error.attemptNumber}`);
console.log(` - Retries left: ${error.retriesLeft}`);
},
}
);
console.log(`✅ All functions deployed successfully`);
} catch (error: any) {
console.error(`❌ Failed to deploy functions:`, error);
// Log detailed error information
if (error.children && error.children.length > 0) {
console.log("📋 Detailed deployment errors:");
error.children.forEach((child: any, index: number) => {
console.log(` ${index + 1}. ${child.message || child}`);
if (child.original) {
console.log(` Original error message: ${child.original.message || "No message"}`);
console.log(` Original error code: ${child.original.code || "No code"}`);
console.log(` Original error status: ${child.original.status || "No status"}`);
}
});
}
// Log the full error structure for debugging
console.log("🔍 Error details:");
console.log(` - Message: ${error.message}`);
console.log(` - Status: ${error.status}`);
console.log(` - Exit code: ${error.exit}`);
console.log(` - Attempt: ${error.attemptNumber}`);
console.log(` - Retries left: ${error.retriesLeft}`);
throw error;
}
const topicName = /\/topics\/([a-zA-Z0-9\-\_]+)/gi.exec(context.resource.name)[1]; | ||
|
||
if (!topicName) { | ||
functions.logger.error( | ||
"Topic name not found in resource name for scheduled function execution" | ||
); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessing [1]
on the result of .exec()
without checking if the match was successful can lead to a TypeError
at runtime if the regex doesn't find a match. This would crash the function.
It's safer to first check if a match was found and then extract the group.
const match = /\/topics\/([\w-]+)/.exec(context.resource.name);
if (!match || !match[1]) {
functions.logger.error(
"Topic name not found in resource name for scheduled function execution"
);
return;
}
const topicName = match[1];
* Deploy functions with rate limiting and retry logic | ||
*/ | ||
export async function deployFunctionsWithRetry( | ||
client: any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | ||
* Post-cleanup: Remove deployed functions after tests | ||
*/ | ||
export async function postCleanup(client: any, testRunId: string): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const sanitizeData = (data: any) => | ||
Object.entries(data).reduce((acc, [key, value]) => { | ||
if (value !== undefined) { | ||
acc[key] = value; | ||
} | ||
return acc; | ||
}, {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation of sanitizeData
works, but it has some typing weaknesses (any
, implicit any
for acc
).
Here's a slightly improved version with better type safety, which makes it more robust and easier to maintain.
export const sanitizeData = (data: any) => | |
Object.entries(data).reduce((acc, [key, value]) => { | |
if (value !== undefined) { | |
acc[key] = value; | |
} | |
return acc; | |
}, {}); | |
export const sanitizeData = (data: Record<string, any>): Record<string, any> => | |
Object.entries(data).reduce((acc: Record<string, any>, [key, value]) => { | |
if (value !== undefined) { | |
acc[key] = value; | |
} | |
return acc; | |
}, {}); |
.onFinalize(async (object: unknown, context) => { | ||
if (!object || typeof object !== "object" || !("name" in object)) { | ||
functions.logger.error("Invalid object structure for storage object finalize"); | ||
return; | ||
} | ||
const name = (object as { name: string }).name; | ||
if (!name || typeof name !== "string") { | ||
functions.logger.error("Invalid name property for storage object finalize"); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The object
parameter is typed as unknown
, which requires manual type guards. It's better to use the ObjectMetadata
type provided by the firebase-functions
SDK for better type safety and code clarity. This avoids the need for manual checks and makes the code more readable.
.onFinalize(async (object: unknown, context) => { | |
if (!object || typeof object !== "object" || !("name" in object)) { | |
functions.logger.error("Invalid object structure for storage object finalize"); | |
return; | |
} | |
const name = (object as { name: string }).name; | |
if (!name || typeof name !== "string") { | |
functions.logger.error("Invalid name property for storage object finalize"); | |
return; | |
} | |
.onFinalize(async (object, context) => { | |
const testId = object.name?.split(".")[0]; | |
if (!testId) { | |
functions.logger.error("Invalid name property for storage object finalize"); | |
return; | |
} |
.onComplete(async (matrix: unknown, context) => { | ||
if (!matrix || typeof matrix !== "object" || !("clientInfo" in matrix)) { | ||
functions.logger.error("Invalid matrix structure for test matrix completion"); | ||
return; | ||
} | ||
const clientInfo = (matrix as { clientInfo: unknown }).clientInfo; | ||
if (!clientInfo || typeof clientInfo !== "object" || !("details" in clientInfo)) { | ||
functions.logger.error("Invalid clientInfo structure for test matrix completion"); | ||
return; | ||
} | ||
const details = clientInfo.details; | ||
if (!details || typeof details !== "object" || !("testId" in details)) { | ||
functions.logger.error("Invalid details structure for test matrix completion"); | ||
return; | ||
} | ||
const testId = details.testId as string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The matrix
parameter is typed as unknown
, leading to a series of manual type checks. You can improve type safety and readability by using the TestMatrix
type from the firebase-functions/v1/testLab
module. This allows you to safely access nested properties with optional chaining.
.onComplete(async (matrix: unknown, context) => { | |
if (!matrix || typeof matrix !== "object" || !("clientInfo" in matrix)) { | |
functions.logger.error("Invalid matrix structure for test matrix completion"); | |
return; | |
} | |
const clientInfo = (matrix as { clientInfo: unknown }).clientInfo; | |
if (!clientInfo || typeof clientInfo !== "object" || !("details" in clientInfo)) { | |
functions.logger.error("Invalid clientInfo structure for test matrix completion"); | |
return; | |
} | |
const details = clientInfo.details; | |
if (!details || typeof details !== "object" || !("testId" in details)) { | |
functions.logger.error("Invalid details structure for test matrix completion"); | |
return; | |
} | |
const testId = details.testId as string; | |
.onComplete(async (matrix, context) => { | |
const testId = matrix.clientInfo?.details?.testId as string; | |
if (!testId) { | |
functions.logger.error("testId not found in test matrix clientInfo"); | |
return; | |
} |
Description
This PR is a rebased version of the stale integration test work.
We have added retries/handling for rate limits on firebase projects
Code sample