tests(Storage): Fix or skip some tests for new CI #15400
Conversation
It's just easier to build this way.
product-auto-label
bot
added
the
api: storage
Summary of ChangesHello @amanda-tarafa, 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 focuses on adapting the Google Cloud Storage C# client's integration tests to a new CI environment. It primarily involves temporarily skipping tests that are currently failing or unstable in the new setup, alongside minor refactorings to test data and IAM policy definitions to enhance test robustness and clarity. Additionally, a new project reference was added to the solution. 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. 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
|
|
Reviewing one commit at a time is best. |
![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 pull request primarily focuses on fixing and skipping integration tests for the Storage client library to improve CI stability. The changes include skipping several tests with references to internal bug reports, which is a reasonable approach for flaky tests. Additionally, some hardcoded strings in DownloadObjectTest have been refactored into constants, which improves code quality. However, I've identified a few issues that need attention. There's a potential bug in the value of a new constant that could cause tests to fail. I also noticed a confusing data change in a skipped test case in NormalizationTest.cs. Finally, an unrelated project reference has been added to the solution file, which seems out of scope for this PR. My detailed comments address these points.
apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.IntegrationTests/StorageFixture.cs
Show resolved
Hide resolved
apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.IntegrationTests/NormalizationTest.cs
Outdated
Show resolved
Hide resolved
amanda-tarafa
force-pushed
the
storage-ci
branch
from
04a88a8 to
b9411bd
Compare
robertvoinescu-work
left a comment
robertvoinescu-work
left a comment
There was a problem hiding this comment.
LGTM: Just one NIT
| // Normalization Form C: a single character for e-acute. | ||
| // URL should end with Caf%C3%A9 | ||
| [InlineData("Caf\u00e9", "Normalization Form C")] | ||
| // Normalization Form D: an ASCII e followed by U+0301 combining character |
There was a problem hiding this comment.
This comment also needs to be updated to match the new data.
There was a problem hiding this comment.
Done, thanks.
See b/477619774.
When we migrated CI, the new gZipped object that was created has cased "Hello World". It's easier to change the tests than to change the object being created via Terraform.
See b/477663109
See b/477676781. Note these were further broken in a previous attempt to fix them at googleapis#14741 and googleapis#14744
amanda-tarafa
force-pushed
the
storage-ci
branch
from
b9411bd to
42234f2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on fixing and skipping tests to stabilize the CI environment. The changes include refactoring hardcoded values in DownloadObjectTest to use constants, which also corrects the expected content of a gzipped file. Several tests across different files (HmacKeysTest, NormalizationTest, PublicAccessPreventionTest, StorageClientSnippets, UrlSignerSnippets) are skipped with corresponding bug tracker IDs. Additionally, some test data and code comments have been updated for correctness, such as changing a member from a specific domain to allUsers.
My main feedback is regarding a change in the solution file that seems out of scope. Otherwise, the changes look good and contribute to improving the test suite's stability and maintainability.
|
(closed by accident) |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on updating and skipping tests for a new CI environment. The changes include refactoring hardcoded values to constants and disabling tests that are currently failing. My review has identified a potential issue with a case-sensitivity change in test data that could lead to test failures, and an unrelated project reference that seems to have been added to the solution file accidentally. Overall, the changes are straightforward, but these two points warrant attention.
apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.IntegrationTests/StorageFixture.cs
Show resolved
Hide resolved
2 participants
No description provided.