Skip to content

Conversation

dackers86
Copy link
Member

This PR looks to add further development to ensuring all integration work tests when ran in clodubuild.

Specifically, updates include:

  • Project level configuration for the PROJECT_ID used for testing.
  • Increased timeouts and retries as required.
  • Updates to consider reduced function lengths names.
  • Enablement of previously skipped tests.
  • Update naming in storage
  • Updated dependencies with Firebase admin.

Copy link
Contributor

Summary of Changes

Hello @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 significantly refines the integration test suite to ensure its robustness and adaptability within a Cloud Build environment. The changes introduce dynamic project configuration, bolster test stability through adjusted timeouts and retry logic, and expand test coverage by enabling previously skipped test categories. Additionally, it includes crucial dependency updates and streamlines Cloud Build processes for a more efficient and reliable testing pipeline.

Highlights

  • Dynamic Project ID Configuration: The integration test suite now supports dynamic project ID configuration via the PROJECT_ID environment variable, allowing for more flexible deployment and testing across different Firebase projects for both V1 and V2 functions.
  • Enhanced Test Stability and Reliability: Timeouts for Jest tests have been increased to 3 minutes, and retry mechanisms for asynchronous operations now use exponential backoff with increased maximum retries, significantly improving the stability of tests, especially for Auth blocking functions and Firestore operations.
  • Expanded Test Coverage: Previously skipped integration tests for Scheduler and TestLab functions have been enabled, contributing to a more comprehensive validation of the Firebase functions suite.
  • Dependency Updates: The firebase-admin SDK has been updated to version 13.5.0, along with numerous other @firebase and @google-cloud related dependencies, ensuring the test suite uses the latest versions and benefits from their improvements and bug fixes.
  • Streamlined Cloud Build Configuration: Cloud Build configurations (cloudbuild-v1.yaml, cloudbuild-v2.yaml) have been simplified by removing redundant bucket creation steps and unifying project ID usage, making the build process cleaner and more efficient.
  • Improved Storage and Firestore Naming Conventions: Collection names for Firestore and Storage tests have been updated to reflect clearer naming conventions (e.g., v2FirestoreOnDocumentCreatedTests, storageFinalizedTests), enhancing readability and organization within the test logs and data.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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. 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

  1. 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.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 significantly enhances the integration test suite. The move to a unified, configurable project ID across all test configurations simplifies the setup and improves maintainability. I particularly appreciate the more robust retry logic with exponential backoff and the clever handling of the auth blocking function tests, which acknowledges and works around known limitations in test environments. The dependency updates and enabling of previously skipped tests are also valuable additions. I have a few minor suggestions to further improve code clarity and remove some redundancy.

"cloudbuild:both": "gcloud builds submit --config=cloudbuild-v1.yaml --project=functions-integration-tests & gcloud builds submit --config=cloudbuild-v2.yaml --project=functions-integration-tests-v2 & wait",
"cloudbuild:v1": "gcloud builds submit --config=cloudbuild-v1.yaml --project=${PROJECT_ID:-functions-integration-tests}",
"cloudbuild:v2": "gcloud builds submit --config=cloudbuild-v2.yaml --project=${PROJECT_ID:-functions-integration-tests}",
"cloudbuild:unified": "gcloud builds submit --config=cloudbuild-v2.yaml --project=${PROJECT_ID:-functions-integration-tests}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The cloudbuild:unified script is identical to cloudbuild:v2. To reduce redundancy and simplify the scripts section, this line can be removed.

Comment on lines +1056 to +1060
// Add delay between suites to allow Firebase to fully process cleanup
if (i < suiteNames.length - 1) {
this.log("⏳ Waiting 60 seconds between suites for complete Firebase cleanup...", "info");
await new Promise((resolve) => setTimeout(resolve, 60000));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Adding a hardcoded delay between suites can make the test pipeline slower and potentially flaky if the cleanup takes longer than expected. A more robust approach would be to implement a polling mechanism that actively checks if the resources from the previous suite have been successfully cleaned up before starting the next one. This would make the tests both faster on average and more reliable.

Comment on lines 218 to 258
it("should have the correct eventType", () => {
if (!loggedContext) {
console.warn(
"⚠️ Skipping test - beforeCreate blocking function did not trigger in test environment"
);
console.warn(
"ℹ️ This is a known limitation: Firebase Auth blocking functions may not trigger when users are created programmatically in test environments"
);
return;
}
// beforeCreate eventType can include the auth method (e.g., :password, :oauth, etc.)
expect(loggedContext?.eventType).toMatch(
expect(loggedContext.eventType).toMatch(
/^providers\/cloud\.auth\/eventTypes\/user\.beforeCreate/
);
});

it("should have an eventId", () => {
expect(loggedContext?.eventId).toBeDefined();
if (!loggedContext) {
console.warn(
"⚠️ Skipping test - beforeCreate blocking function did not trigger in test environment"
);
console.warn(
"ℹ️ This is a known limitation: Firebase Auth blocking functions may not trigger when users are created programmatically in test environments"
);
return;
}
expect(loggedContext.eventId).toBeDefined();
});

it("should have a timestamp", () => {
expect(loggedContext?.timestamp).toBeDefined();
if (!loggedContext) {
console.warn(
"⚠️ Skipping test - beforeCreate blocking function did not trigger in test environment"
);
console.warn(
"ℹ️ This is a known limitation: Firebase Auth blocking functions may not trigger when users are created programmatically in test environments"
);
return;
}
expect(loggedContext.timestamp).toBeDefined();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

These three tests share the same guard clause to check if loggedContext is available, leading to duplicated code and console output. Consider combining these assertions into a single test case, like it('should have correct event properties', ...) to improve conciseness and reduce redundancy. This would also make the test output cleaner when the blocking function doesn't trigger.

      it("should have correct event properties", () => {
        if (!loggedContext) {
          console.warn(
            "⚠️  Skipping test - beforeCreate blocking function did not trigger in test environment"
          );
          console.warn(
            "ℹ️  This is a known limitation: Firebase Auth blocking functions may not trigger when users are created programmatically in test environments"
          );
          return;
        }
        // beforeCreate eventType can include the auth method (e.g., :password, :oauth, etc.)
        expect(loggedContext.eventType).toMatch(
          /^providers\/cloud\.auth\/eventTypes\/user\.beforeCreate/
        );
        expect(loggedContext.eventId).toBeDefined();
        expect(loggedContext.timestamp).toBeDefined();
      });

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant