ROSAENG-60112 | test: add unit tests for pkg/aws#3274
Draft
olucasfreitas wants to merge 2 commits into
Draft
Conversation
Add meaningful unit tests for previously untested files in pkg/aws: - idps_test.go: OIDC provider Create/Has/Delete operations - quota_test.go: service quota lookup and IAM quota retrieval - policy_document_test.go: policy parsing, interpolation, principals, actions - helpers_additional_test.go: ARN validation, path extraction, subnet display - cloudformation_test.go: stack lifecycle, ready check, update no-op - sts_test.go: account ID validation, permissions boundary, role sorting Total: 6 new files, ~120 test cases, all passing.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughSeven new Ginkgo/Gomega test files are added to 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olucasfreitas 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 |
olucasfreitas
changed the title
openshift-ci
Bot
added
the
approved
Contributor
Author
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
Contributor
Author
|
/test all |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Add tests for previously uncovered high-priority functions: - client_additional_test.go: CheckRoleExists, GetRoleByARN, GetRoleByName, CreateSecretInSecretsManager, DeleteSecretInSecretsManager - policies_additional_test.go: hasCompatibleMajorMinorVersionTags (semver upgrade logic), IsPolicyCompatible (version tag checking) - sts_test.go: DeleteUserRole, DeleteOCMRole with full mock chains including detach/delete policy flows and DeleteConflictException handling - helpers_additional_test.go: ARNPathValidator, UserNoProxyValidator, UserNoProxyDuplicateValidator, GetOCMRoleName, GetUserRoleName, GetOperatorPolicyName, GetPolicyArn, GetOperatorPolicyARN, IsStandardNamedAccountRole, isSTS, resolveSTSRole Total: 309 specs (up from 121 baseline), all passing.
Contributor
Author
|
/test all |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/aws/client_additional_test.go (1)
202-205: ⚡ Quick winStrengthen Secrets Manager input assertions to avoid false positives.
These expectations currently accept any request payload, so regressions in secret target/name/value can still pass. Assert key input fields in the mock callbacks.
Suggested tightening
- mockSecretsManagerAPI.EXPECT().CreateSecret(gomock.Any(), gomock.Any()).Return( - &secretsmanager.CreateSecretOutput{ - ARN: awsSdk.String(secretARN), - }, nil) + mockSecretsManagerAPI.EXPECT().CreateSecret(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ interface{}, input *secretsmanager.CreateSecretInput, _ ...interface{}) (*secretsmanager.CreateSecretOutput, error) { + Expect(awsSdk.ToString(input.Name)).To(Equal(secretName)) + Expect(awsSdk.ToString(input.SecretString)).To(Equal(secretValue)) + return &secretsmanager.CreateSecretOutput{ + ARN: awsSdk.String(secretARN), + }, nil + }) - mockSecretsManagerAPI.EXPECT().DescribeSecret(gomock.Any(), gomock.Any()).Return( - &secretsmanager.DescribeSecretOutput{}, nil) - mockSecretsManagerAPI.EXPECT().DeleteSecret(gomock.Any(), gomock.Any()).Return( - &secretsmanager.DeleteSecretOutput{}, nil) + mockSecretsManagerAPI.EXPECT().DescribeSecret(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ interface{}, input *secretsmanager.DescribeSecretInput, _ ...interface{}) (*secretsmanager.DescribeSecretOutput, error) { + Expect(awsSdk.ToString(input.SecretId)).To(Equal(secretARN)) + return &secretsmanager.DescribeSecretOutput{}, nil + }) + mockSecretsManagerAPI.EXPECT().DeleteSecret(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ interface{}, input *secretsmanager.DeleteSecretInput, _ ...interface{}) (*secretsmanager.DeleteSecretOutput, error) { + Expect(awsSdk.ToString(input.SecretId)).To(Equal(secretARN)) + return &secretsmanager.DeleteSecretOutput{}, nil + })As per coding guidelines,
**/*_test.go: “Flag weak tests that only restate implementation or changes that weaken existing assertions.”Also applies to: 230-233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/aws/client_additional_test.go` around lines 202 - 205, The mock expectation for mockSecretsManagerAPI.EXPECT().CreateSecret currently uses gomock.Any() to match the input parameter, which means any request payload is accepted and regressions in secret target/name/value can pass undetected. Replace the gomock.Any() matcher for the input parameter in the CreateSecret expectation with specific matchers (such as gomock.Eq() or a custom matcher) that assert the key input fields like the secret name and value being passed to the API call. This will ensure the test catches actual regressions if these critical parameters are not set correctly. Apply the same strengthening to the other CreateSecret expectation mentioned in the comment.Source: Coding guidelines
pkg/aws/sts_test.go (1)
300-301: ⚡ Quick winAssert role/policy identifiers in detach/delete expectations.
Using
gomock.Any()+ call counts here allows wrong policy ARN/role wiring to pass. Use explicitDetachRolePolicyInput/DeletePolicyInputexpectations per policy.Suggested tightening
- mockIamAPI.EXPECT().DetachRolePolicy(gomock.Any(), gomock.Any()).Return(&iam.DetachRolePolicyOutput{}, nil).Times(2) + mockIamAPI.EXPECT().DetachRolePolicy(gomock.Any(), &iam.DetachRolePolicyInput{ + RoleName: awsSdk.String("test-role"), + PolicyArn: awsSdk.String("arn:aws:iam::123:policy/Policy1"), + }).Return(&iam.DetachRolePolicyOutput{}, nil) + mockIamAPI.EXPECT().DetachRolePolicy(gomock.Any(), &iam.DetachRolePolicyInput{ + RoleName: awsSdk.String("test-role"), + PolicyArn: awsSdk.String("arn:aws:iam::123:policy/Policy2"), + }).Return(&iam.DetachRolePolicyOutput{}, nil) - mockIamAPI.EXPECT().DetachRolePolicy(gomock.Any(), gomock.Any()).Return(&iam.DetachRolePolicyOutput{}, nil) - mockIamAPI.EXPECT().DeletePolicy(gomock.Any(), gomock.Any()).Return(&iam.DeletePolicyOutput{}, nil) + mockIamAPI.EXPECT().DetachRolePolicy(gomock.Any(), &iam.DetachRolePolicyInput{ + RoleName: awsSdk.String("ocm-role"), + PolicyArn: awsSdk.String("arn:aws:iam::123:policy/TestPolicy"), + }).Return(&iam.DetachRolePolicyOutput{}, nil) + mockIamAPI.EXPECT().DeletePolicy(gomock.Any(), &iam.DeletePolicyInput{ + PolicyArn: awsSdk.String("arn:aws:iam::123:policy/TestPolicy"), + }).Return(&iam.DeletePolicyOutput{}, nil)As per coding guidelines,
**/*_test.go: “Flag weak tests that only restate implementation or changes that weaken existing assertions.”Also applies to: 391-392
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/aws/sts_test.go` around lines 300 - 301, The mock expectations for DetachRolePolicy and DeletePolicy use gomock.Any() for input parameters combined with Times(2) call counts, which weakens the test by allowing incorrect policy ARNs or role names to pass without detection. Replace the gomock.Any() placeholders with explicit DetachRolePolicyInput and DeletePolicyInput expectations that specify the exact policy ARN and role identifier values that should be used, ensuring the test validates the correct wiring of roles and policies rather than just counting calls.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/aws/policy_document_test.go`:
- Around line 67-109: The GetAWSPrincipals test suite does not cover the edge
case where the PolicyStatement has a nil Principal field. Add a new test case
within the Describe("GetAWSPrincipals") block that creates a PolicyStatement
with Principal explicitly set to nil and verifies that GetAWSPrincipals()
handles this gracefully by returning an empty slice, ensuring the implementation
does not panic when dereferencing a nil Principal.
---
Nitpick comments:
In `@pkg/aws/client_additional_test.go`:
- Around line 202-205: The mock expectation for
mockSecretsManagerAPI.EXPECT().CreateSecret currently uses gomock.Any() to match
the input parameter, which means any request payload is accepted and regressions
in secret target/name/value can pass undetected. Replace the gomock.Any()
matcher for the input parameter in the CreateSecret expectation with specific
matchers (such as gomock.Eq() or a custom matcher) that assert the key input
fields like the secret name and value being passed to the API call. This will
ensure the test catches actual regressions if these critical parameters are not
set correctly. Apply the same strengthening to the other CreateSecret
expectation mentioned in the comment.
In `@pkg/aws/sts_test.go`:
- Around line 300-301: The mock expectations for DetachRolePolicy and
DeletePolicy use gomock.Any() for input parameters combined with Times(2) call
counts, which weakens the test by allowing incorrect policy ARNs or role names
to pass without detection. Replace the gomock.Any() placeholders with explicit
DetachRolePolicyInput and DeletePolicyInput expectations that specify the exact
policy ARN and role identifier values that should be used, ensuring the test
validates the correct wiring of roles and policies rather than just counting
calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f3221126-1f62-42d2-86ca-6c7786a4b68a
📒 Files selected for processing (8)
pkg/aws/client_additional_test.gopkg/aws/cloudformation_test.gopkg/aws/helpers_additional_test.gopkg/aws/idps_test.gopkg/aws/policies_additional_test.gopkg/aws/policy_document_test.gopkg/aws/quota_test.gopkg/aws/sts_test.go
Comment on lines
+67
to
+109
| Describe("GetAWSPrincipals", func() { | ||
| It("returns a single principal when AWS is a string", func() { | ||
| stmt := PolicyStatement{ | ||
| Principal: &PolicyStatementPrincipal{ | ||
| AWS: "arn:aws:iam::123456:root", | ||
| }, | ||
| } | ||
| Expect(stmt.GetAWSPrincipals()).To(Equal([]string{"arn:aws:iam::123456:root"})) | ||
| }) | ||
|
|
||
| It("returns all principals when AWS is a slice", func() { | ||
| stmt := PolicyStatement{ | ||
| Principal: &PolicyStatementPrincipal{ | ||
| AWS: []interface{}{"arn:aws:iam::111:root", "arn:aws:iam::222:root"}, | ||
| }, | ||
| } | ||
| Expect(stmt.GetAWSPrincipals()).To(ConsistOf( | ||
| "arn:aws:iam::111:root", | ||
| "arn:aws:iam::222:root", | ||
| )) | ||
| }) | ||
|
|
||
| It("returns an empty slice when AWS is nil", func() { | ||
| stmt := PolicyStatement{ | ||
| Principal: &PolicyStatementPrincipal{AWS: nil}, | ||
| } | ||
| Expect(stmt.GetAWSPrincipals()).To(BeEmpty()) | ||
| }) | ||
|
|
||
| It("handles a single-element slice from unmarshalled JSON", func() { | ||
| raw := `{"Principal":{"AWS":["arn:aws:iam::999:role/foo"]}}` | ||
| var stmt PolicyStatement | ||
| Expect(json.Unmarshal([]byte(raw), &stmt)).To(Succeed()) | ||
| Expect(stmt.GetAWSPrincipals()).To(Equal([]string{"arn:aws:iam::999:role/foo"})) | ||
| }) | ||
|
|
||
| It("handles a string from unmarshalled JSON", func() { | ||
| raw := `{"Principal":{"AWS":"arn:aws:iam::888:role/bar"}}` | ||
| var stmt PolicyStatement | ||
| Expect(json.Unmarshal([]byte(raw), &stmt)).To(Succeed()) | ||
| Expect(stmt.GetAWSPrincipals()).To(Equal([]string{"arn:aws:iam::888:role/bar"})) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Add a nil-Principal test for GetAWSPrincipals.
This suite never exercises PolicyStatement{Principal: nil}. In pkg/aws/policy_document.go, GetAWSPrincipals dereferences p.Principal directly, so this edge case can panic untested.
✅ Suggested test addition
Describe("GetAWSPrincipals", func() {
+ It("returns an empty slice when Principal is nil", func() {
+ stmt := PolicyStatement{}
+ Expect(stmt.GetAWSPrincipals()).To(BeEmpty())
+ })
+
It("returns a single principal when AWS is a string", func() {
stmt := PolicyStatement{
Principal: &PolicyStatementPrincipal{As per coding guidelines, “tests should prove correctness and be kept accurate to real failure/edge cases.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/aws/policy_document_test.go` around lines 67 - 109, The GetAWSPrincipals
test suite does not cover the edge case where the PolicyStatement has a nil
Principal field. Add a new test case within the Describe("GetAWSPrincipals")
block that creates a PolicyStatement with Principal explicitly set to nil and
verifies that GetAWSPrincipals() handles this gracefully by returning an empty
slice, ensuring the implementation does not panic when dereferencing a nil
Principal.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add meaningful unit tests for previously untested files and functions in
pkg/aws/. This is Phase 1A of the coverage improvement initiative targeting 90% meaningful test coverage across the ROSA CLI.New and expanded test files (8 files, ~185 test cases, 2,645 lines):
idps_test.goquota_test.gopolicy_document_test.gohelpers_additional_test.gocloudformation_test.gosts_test.goclient_additional_test.gopolicies_additional_test.goWhat's NOT included (intentionally):
permissions.go(ValidateSCP) -- deep dependencies onSimulatePrincipalPolicypaginator; better covered by integration/E2E testsValidateQuota/ListServiceQuotas-- use internal paginators that can't be cleanly mocked; the pureGetServiceQuotafunction is testedCreateS3Bucket/DeleteS3Bucket-- require 5+ chained S3 mock expectations; meaningful testing requires integration approachSummary:
pkg/awsValidation
go test ./pkg/aws/...-- 309/309 specs passgo vet ./pkg/aws/...-- cleanmake pre-push-checks-- all 5 checks pass (format, build, lint, coverage, tests)Links
Summary by CodeRabbit