Skip to content

[Vertex AI] Add integration tests for staging endpoint #14637

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

Merged
merged 3 commits into from
Apr 1, 2025

Conversation

andrewheard
Copy link
Contributor

Updated CountTokensIntegrationTests and GenerateContentIntegrationTests to test against the staging endpoint in addition to the existing prod tests. This will allow us to catch any regressions before backend builds reach prod.

#no-changelog

This comment was marked as resolved.

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 introduces integration tests for the staging endpoint in addition to the existing prod tests. This is a good approach to catch regressions before backend builds reach prod. The changes look good overall, but I have a few suggestions for improvement.

Summary of Findings

  • APIConfig enum case: The vertexAI case in the APIConfig enum now takes an endpoint parameter. This is a good change to support different endpoints. However, it might be beneficial to consider a more descriptive name for the enum case to improve clarity.
  • Assertions in VertexAI.swift: The assertions in the VertexAI.swift file now explicitly check for the .vertexAI(endpoint: .firebaseVertexAIProd) endpoint. While this is correct, it might be beneficial to add a comment explaining why these assertions are necessary and what they are verifying.
  • InstanceConfig definitions: The InstanceConfig struct in InstanceConfig.swift now includes definitions for both prod and staging endpoints. This is a good approach for testing against different environments. However, it might be beneficial to add documentation or comments explaining the purpose of each configuration.

Merge Readiness

The pull request is well-structured and introduces a valuable addition to the testing suite. I recommend addressing the comments provided to enhance code clarity and maintainability. Once these suggestions are incorporated, the pull request will be ready for merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging.

@andrewheard andrewheard marked this pull request as ready for review April 1, 2025 18:15
@andrewheard andrewheard requested a review from paulb777 April 1, 2025 18:15
@andrewheard
Copy link
Contributor Author

cc: @rainshen49

@andrewheard andrewheard merged commit c4a4550 into main Apr 1, 2025
37 checks passed
@andrewheard andrewheard deleted the ah/vertex-integration-staging branch April 1, 2025 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants