oadp-1.5: Fix S3 bucket region unit tests by adding mocking support (#2052) #2071
Conversation
…2052) The unit tests for S3 bucket region auto-discovery were making real network calls to AWS, causing failures in CI environments without network access to the test buckets. Changes: - Add GetBucketRegionFunc variable in pkg/storage/aws/s3.go to enable mocking in tests - Refactor s3_test.go to use mocks instead of real AWS calls - Add mock setup in bsl_test.go for tests using DiscoverableBucket - Add mock setup in common_test.go for TestUpdateBackupStorageLocation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Comment |
openshift-ci
bot
added
the
approved
There was a problem hiding this comment.
Pull request overview
This PR fixes unit tests for S3 bucket region auto-discovery that were making real network calls to AWS, causing failures in CI environments without network access. The solution introduces a mockable function variable to enable testing without actual AWS API calls.
Changes:
- Add mocking infrastructure to pkg/storage/aws/s3.go with GetBucketRegionFunc variable
- Refactor s3_test.go to use mocks instead of real AWS API calls
- Add mock setup in test files where bucket region discovery is used
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/storage/aws/s3.go | Introduces GetBucketRegionFunc variable for mocking and refactors GetBucketRegion to delegate to it |
| pkg/storage/aws/s3_test.go | Updates tests to use mocks instead of real AWS buckets, improving test reliability and removing network dependencies |
| pkg/common/common_test.go | Adds mock setup for GetBucketRegionFunc (though this appears unnecessary for the function being tested) |
| internal/controller/bsl_test.go | Adds mock setup to return region for DiscoverableBucket constant used in test cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Mock GetBucketRegionFunc to return a region for the test bucket | ||
| originalGetBucketRegionFunc := aws.GetBucketRegionFunc | ||
| aws.GetBucketRegionFunc = func(bucket string) (string, error) { | ||
| if bucket == "openshift-velero-plugin-s3-auto-region-test-1" { | ||
| return "us-east-1", nil | ||
| } | ||
| return "", errors.New("bucket region not discoverable") | ||
| } | ||
| defer func() { aws.GetBucketRegionFunc = originalGetBucketRegionFunc }() |
There was a problem hiding this comment.
The mock setup for GetBucketRegionFunc appears unnecessary in this test. The UpdateBackupStorageLocation function being tested only calls aws.StripDefaultPorts() and does not invoke any bucket region discovery functionality. The mock checks for a bucket name "openshift-velero-plugin-s3-auto-region-test-1" that doesn't appear in any of the test cases in this test function. This mock should be removed as it adds confusion without providing value.
| // Mock GetBucketRegionFunc to return a region for the test bucket | |
| originalGetBucketRegionFunc := aws.GetBucketRegionFunc | |
| aws.GetBucketRegionFunc = func(bucket string) (string, error) { | |
| if bucket == "openshift-velero-plugin-s3-auto-region-test-1" { | |
| return "us-east-1", nil | |
| } | |
| return "", errors.New("bucket region not discoverable") | |
| } | |
| defer func() { aws.GetBucketRegionFunc = originalGetBucketRegionFunc }() |
| // GetBucketRegion returns the AWS region that a bucket is in, or an error | ||
| // if the region cannot be determined. | ||
| // if the region cannot be determined. This is a wrapper that calls GetBucketRegionFunc. | ||
| func GetBucketRegion(bucket string) (string, error) { | ||
| return GetBucketRegionFunc(bucket) | ||
| } |
There was a problem hiding this comment.
The comment describes GetBucketRegion as "a wrapper that calls GetBucketRegionFunc", which is accurate but could be clearer. Since GetBucketRegion is the public API function that was refactored to enable mocking, consider updating the comment to clarify its purpose, such as: "GetBucketRegion returns the AWS region that a bucket is in, or an error if the region cannot be determined. It delegates to GetBucketRegionFunc which can be mocked in tests."
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, shubham-pampattiwar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@kaovilai: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
cp: #2052
The unit tests for S3 bucket region auto-discovery were making real
network calls to AWS, causing failures in CI environments without
network access to the test buckets.
Changes:
mocking in tests
🤖 Generated with Claude Code
Co-authored-by: Claude Opus 4.5 noreply@anthropic.com
Why the changes were made
How to test the changes made