ROSAENG-60112 | test: add focused coverage for untested pkg packages#3277
Conversation
…(Phase 1C) Add meaningful tests for previously untested package-level behavior: - pkg/fedramp: flag wiring, env validation, config-backed enable/disable behavior - pkg/input: YAML/JSON file loading and hypershift happy path - pkg/logging: logger builders, round tripper validation, JSON/form redaction - pkg/color: flag defaults, completion, explicit color modes - pkg/aws/profile: flag/env precedence - pkg/aws/region: flag/env precedence and escaped-empty handling - pkg/debug: debug flag state - pkg/clients: HTTP client wrapper behavior Skip constants-only packages and avoid low-value coverage-for-coverage-sake tests. Validation: - go test ./pkg/fedramp ./pkg/input ./pkg/logging ./pkg/color ./pkg/aws/profile ./pkg/aws/region ./pkg/debug ./pkg/clients -count=1 - go test ./pkg/fedramp ./pkg/input ./pkg/logging ./pkg/color ./pkg/aws/profile ./pkg/aws/region ./pkg/debug ./pkg/clients -cover - go vet ./pkg/fedramp ./pkg/input ./pkg/logging ./pkg/color ./pkg/aws/profile ./pkg/aws/region ./pkg/debug ./pkg/clients
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughEight new Ginkgo/Gomega test files were added across 🚥 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 |
openshift-ci
Bot
added
the
approved
olucasfreitas
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3277 +/- ##
==========================================
+ Coverage 25.00% 27.02% +2.02%
==========================================
Files 330 334 +4
Lines 36162 36704 +542
==========================================
+ Hits 9042 9920 +878
+ Misses 26458 26029 -429
- Partials 662 755 +93 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| Describe("CheckIfHypershiftClusterOrExit", func() { | ||
| It("returns without exiting for a hypershift cluster", func() { | ||
| cluster, err := cmv1.NewCluster(). | ||
| Hypershift(cmv1.NewHypershift().Enabled(true)). |
There was a problem hiding this comment.
What is the expected behavior if is not enabled?
There was a problem hiding this comment.
The non-HCP path calls os.Exit(1) directly, so testing it at unit level would need subprocess tricks or a refactor of the function signature, both out of scope here. The happy path test documents the intended behavior for HCP clusters; the exit behavior is better validated at the command integration level.
| Describe("IsValidEnv", func() { | ||
| It("recognizes known environments", func() { | ||
| Expect(IsValidEnv("production")).To(BeTrue()) | ||
| Expect(IsValidEnv("staging")).To(BeTrue()) |
There was a problem hiding this comment.
Is staging01 a valid env too?
There was a problem hiding this comment.
Good catch, it is! Adding it to the assertion.
|
|
||
| Expect(Region()).To(Equal("")) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Would make sense having one "returns an empty string when neither flag nor environment is set" like the profile has?
There was a problem hiding this comment.
Good call, adding it for parity with the profile tests.
| }) | ||
|
|
||
| Describe("RoundTripper", func() { | ||
| It("preserves request and response bodies and redacts JSON fields in logs", func() { |
There was a problem hiding this comment.
If this test fails how do we know the cause? bodies not preserved or json fields not redacted?
There was a problem hiding this comment.
Agree, splitting it into three focused tests so a failure tells you exactly what broke.
|
|
||
| Describe("Disable", func() { | ||
| It("clears the in-memory flag and persists FedRAMP=false for a valid config", func() { | ||
| tempDir := GinkgoT().TempDir() |
There was a problem hiding this comment.
minor: What is the difference between this and the os.MkdirTemp?
There was a problem hiding this comment.
No real difference in outcome, GinkgoT().TempDir() is just Ginkgo's managed version that auto-cleans after the test. I was mixing the two approaches, standardizing all of them to GinkgoT().TempDir() now.
| stdout, err := os.Stdout.Stat() | ||
| expected := true | ||
| if err == nil { | ||
| expected = (stdout.Mode()&os.ModeDevice != 0) && (stdout.Mode()&os.ModeNamedPipe == 0) |
There was a problem hiding this comment.
The expected value is computed with the same stdout logic as UseColor(), right?
If this test failed, what would tell?
I would suggest dropping it and relying on never/always + the unknown→default case (and reporter tests) for color coverage. wdyt?
There was a problem hiding this comment.
You're right, it's basically testing the function against itself. Dropping it. The never, always, and unknown-falls-to-default cases already cover the meaningful paths.
| Expect(HasFlag(cmd)).To(BeFalse()) | ||
| Expect(HasAdminFlag(cmd)).To(BeFalse()) | ||
|
|
||
| Expect(cmd.Flags().Set("govcloud", "true")).To(Succeed()) |
There was a problem hiding this comment.
-
With this test I noticed that we have 2 flags (--admin and --govcloud) that enables fedramp. Is this expected? (out of scope of this PR but nice to review since we are here ;-) )
-
If this is expected then should we have a test to validate behavior if just one of them are enabled?
There was a problem hiding this comment.
- Yeah, both flags bind to the same
enabledvar, that's existing behavior. Worth reviewing separately but out of scope here. 2) Good idea, adding a test that setting one flag doesn't make the other one report as changed.
| Expect(Enabled()).To(BeFalse()) | ||
| }) | ||
|
|
||
| It("returns false when the config file does not exist", func() { |
There was a problem hiding this comment.
Would make sense covering the cases below? (if they are real cases)
- Enabled() with valid config + FedRAMP: false
- Disable() when config is missing
- Disable() when config is invalid
There was a problem hiding this comment.
Good suggestions, they are real cases. Adding all three.
- Add staging01 to IsValidEnv assertion - Add region empty-string test for parity with profile tests - Split round-tripper test into three focused tests (body preservation, JSON redaction, auth header omission) - Drop tautological auto-color test - Add flag-independence tests for govcloud vs admin - Add edge cases: Enabled with FedRAMP:false, Disable with missing and invalid config
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amandahla, 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: 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
PR Summary
Add focused, meaningful unit tests for previously untested package-level behavior in the early-priority Phase 1C scope. This aligns the change with the plan's first implementation step instead of the earlier pkg/aws Phase 1A direction.
Detailed Description of the Issue
The coverage plan prioritizes untested
pkg/packages first because they are fast, reviewable wins that raise confidence without over-investing in low-value command wiring. Several package-level utilities and integration helpers had no automated coverage at all, including FedRAMP config behavior, input file parsing, logging helpers, color/profile/region/debug flag behavior, and the small HTTP client wrapper.This PR adds focused tests for the real behavior in those packages while intentionally skipping constants-only packages and avoiding coverage-for-coverage-sake assertions.
Related Issues and PRs
#Type of Change
Previous Behavior
The following packages had no dedicated automated coverage for their package-level behavior:
pkg/fedramppkg/inputpkg/loggingpkg/colorpkg/aws/profilepkg/aws/regionpkg/debugpkg/clientsBehavior After This Change
The repo now has focused tests for:
Intentionally not added:
pkg/info,pkg/object,pkg/properties, andpkg/constantsHow to Test (Step-by-Step)
Preconditions
Test Steps
go test ./pkg/fedramp ./pkg/input ./pkg/logging ./pkg/color ./pkg/aws/profile ./pkg/aws/region ./pkg/debug ./pkg/clients -count=1go test ./pkg/fedramp ./pkg/input ./pkg/logging ./pkg/color ./pkg/aws/profile ./pkg/aws/region ./pkg/debug ./pkg/clients -covergo vet ./pkg/fedramp ./pkg/input ./pkg/logging ./pkg/color ./pkg/aws/profile ./pkg/aws/region ./pkg/debug ./pkg/clientsExpected Results
go vetfinishes without findingsProof of the Fix
go testpasses for all targeted packagesgo vetis cleanBreaking Changes
Breaking Change Details / Migration Plan
N/A
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Summary by CodeRabbit
profileandregionflag + environment variable precedence and special empty-value handling.coloranddebugflag tests for default behavior and state changes.