-
Notifications
You must be signed in to change notification settings - Fork 213
V2 integration tests #1717
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
V2 integration tests #1717
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!
I've undertaken a significant overhaul of our integration test suite, aiming to modernize and streamline our testing processes. The core of this change involves migrating from our existing shell-scripted, in-function test orchestration to a more robust and maintainable Jest-based framework. This PR introduces a new TypeScript-driven test runner that intelligently discovers and deploys functions, executes tests across various Firebase services for both v1 and v2 Cloud Functions, and ensures proper cleanup. This comprehensive update centralizes test logging to Firestore and enhances the overall configurability of our test environments, providing a clearer and more efficient way to validate our function deployments.
Highlights
- Migration to Jest Testing Framework: I've completely refactored the integration test suite, moving from a custom shell script and in-function test orchestration to a modern Jest-based framework. This change significantly improves test structure, readability, and maintainability.
- New TypeScript-based Test Runner: I've introduced a new TypeScript-based runner (
run.ts
) that orchestrates the entire integration test lifecycle, including dynamic endpoint discovery, function deployment, test execution, and comprehensive cleanup. This replaces the previous manual and less robust shell scripts. - Comprehensive V1 and V2 Function Integration Tests: I've added extensive integration tests for both v1 and v2 Firebase Cloud Functions across a wide array of services, including Auth, Realtime Database, Firestore, Pub/Sub, Remote Config, Storage, Cloud Tasks, Test Lab, Alerts, Eventarc, Identity, and Scheduler.
- Centralized Test Logging to Firestore: Test results and event contexts are now centrally logged to Firestore, providing a unified and easily queryable source for test outcomes and debugging information.
- Enhanced Test Environment Configurability: The test environment is now more configurable, leveraging environment variables (loaded from
.env
) and dynamic region selection, making it easier to adapt tests to different project setups and deployment targets.
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
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 pull request introduces a comprehensive integration test suite for V2 functions, refactoring the existing tests and adding many new ones. The changes are well-structured, moving from a custom test runner to Jest and organizing tests by function version and type. The new test runner script (run.ts
) is a significant improvement over the old shell script. My feedback includes suggestions to improve configuration portability, type safety, and robustness in a few utility functions, as well as a critical fix for potential runtime errors.
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 the result of exec
with [1]
is unsafe and will throw a TypeError
if the regular expression does not find a match. This would crash the function. It's much safer to check for the existence of a match before accessing captured groups.
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; | |
} | |
const match = /\/topics\/([a-zA-Z0-9\-\_]+)/gi.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]; |
id: "Set Project ID" | ||
dir: "integration_test" | ||
entrypoint: "npx" | ||
args: ["firebase", "use", "cf3-integration-tests-d7be6"] |
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 project ID cf3-integration-tests-d7be6
is hardcoded. This makes the build configuration less portable. It's better to use a substitution variable (e.g., $_PROJECT_ID
or $PROJECT_ID
) which can be provided at build time. This allows the same configuration to be used for different projects or environments.
FIREBASE_MEASUREMENT_ID= | ||
FIREBASE_AUTH_DOMAIN= | ||
FIREBASE_API_KEY= | ||
GOOGLE_ANALYTICS_API_SECRET= |
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.
<?xml version="1.0" encoding="UTF-8"?> | ||
<module type="WEB_MODULE" version="4"> | ||
<component name="NewModuleRootManager" inherit-compiler-output="true"> | ||
<exclude-output /> | ||
<content url="file://$MODULE_DIR$" /> | ||
<orderEntry type="inheritedJdk" /> | ||
<orderEntry type="sourceFolder" forTests="false" /> | ||
</component> | ||
</module> |
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.
The sanitizeData
function can be made more type-safe. The accumulator acc
in the reduce
function is not explicitly typed, which can lead to issues with strict type checking, especially since noImplicitAny
is enabled. Providing a generic type argument to reduce
will improve type safety.
export const sanitizeData = (data: any) => | |
Object.entries(data).reduce((acc, [key, value]) => { | |
if (value !== undefined) { | |
acc[key] = value; | |
} | |
return acc; | |
}, {}); | |
export const sanitizeData = (data: any) => | |
Object.entries(data).reduce<Record<string, any>>((acc, [key, value]) => { | |
if (value !== undefined) { | |
acc[key] = value; | |
} | |
return acc; | |
}, {}); |
export function loadEnv(): void { | ||
try { | ||
const envPath = path.resolve(process.cwd(), ".env"); | ||
console.log("Loading .env file from", envPath); | ||
const envFileContent = fs.readFileSync(envPath, "utf-8"); | ||
envFileContent.split("\n").forEach((variable) => { | ||
const [key, value] = variable.split("="); | ||
if (key && value) process.env[key.trim()] = value.trim(); | ||
}); | ||
} catch (error: any) { | ||
console.error("Error loading .env file:", error.message); | ||
} | ||
} |
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 custom loadEnv
function is a simple implementation that might not handle all edge cases in .env
files, such as comments or quoted values. For a more robust and standard solution, consider using a well-tested library like dotenv
. You can add it as a dev dependency and call dotenv.config()
at the beginning of your script.
This is just a draft PR to compare and investigate changes