Skip to content

test: improve Go test idioms across the codebase [RHIDP-14400] - #421

Open
rm3l wants to merge 2 commits into
redhat-developer:mainfrom
rm3l:test/improve-go-test-idioms
Open

rm3l wants to merge 2 commits into
redhat-developer:mainfrom
rm3l:test/improve-go-test-idioms

Conversation

@rm3l

@rm3l rm3l commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

Description

Improve Go test idiomaticness across the codebase:

  • Add missing test files for clusterinfo.go, route.go, output.go, and collector.go — covering writeYAML, ClusterInfo.Run, getString, Route.Run (with/without Route API), setGVK, writeResource, writeCollectError, knownGroupKinds, listResourceNames, resolveCRDType, Config.IsInterrupted, Config.Namespaces, Config.ShouldInclude, Config.ApplyLogSince, and Registry completeness
  • Add t.Run subtests to all table-driven tests that were missing them (TestIsRHDHRelated, TestMatchesInstance, TestHumanSize, TestMatchesAnyPattern, TestOwnerRefKind, TestHeapDumpMethodValidation, TestIsSecretDocument) for precise failure diagnostics
  • Extract shared test helper (testhelper_test.go) with functional options (withAPIGroups, withTypedObjs, withDynamicObjs, withInterrupted), replacing duplicated fake-client setup across platform_test.go, clusterinfo_test.go, route_test.go, and output_test.go
  • Unify duplicate factory helpers in kube/client_test.go (newTestClient + newTestClientWithVersions → single newTestClient) and consolidate TestHasAPIGroup_* into a table-driven test
  • Consolidate individual test functions in namespace/filter_test.go (12 functions → 4 table-driven tests)

Which issue(s) does this PR fix or relate to

PR acceptance criteria

  • Tests
  • Documentation

How to test changes / Special notes to the reviewer

All changes are test-only. Run make test and make lint — both pass cleanly. No production code was modified.

🤖 Generated with Claude Code

Add missing test files for untested source files (clusterinfo.go,
route.go, output.go, collector.go), add t.Run subtests to all
table-driven tests that were missing them for better failure
diagnostics, consolidate duplicate test helpers into a shared
testhelper_test.go with functional options, unify the two nearly
identical factory helpers in kube/client_test.go, and consolidate
12 individual test functions in namespace/filter_test.go into 4
table-driven tests.

Assisted-by: Claude
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Oct 2, 2026
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

PR images are available (for 1 week):

  1. quay.io/rhdh-community/rhdh-must-gather:pr-421
  2. quay.io/rhdh-community/rhdh-must-gather:pr-421-94e4889ff

@rm3l
rm3l marked this pull request as ready for review October 2, 2026 08:37
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Oct 2, 2026
@rhdh-qodo-merge

Copy link
Copy Markdown

PR Summary by Qodo

Expand Go test coverage and standardize table-driven tests

🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Add coverage for collector configuration, cluster information, routes, and output helpers.
• Share fake Kubernetes client setup across collector tests to reduce duplication.
• Give table-driven cases named subtests and consolidate overlapping test fixtures.
Diagram

graph TD
  CollectorTests["Collector tests"] --> SharedFixture["Shared fixture"] --> FakeKube[("Fake Kubernetes")]
  CollectorTests --> CollectorLogic["Collector behavior"] --> OutputFiles["Temporary outputs"]
  KubeTests["Kube tests"] --> FakeKube
  NamespaceTests["Namespace tests"] --> NamespaceLogic["Namespace filtering"]
Loading
High-Level Assessment

The shared fake-client fixture with functional options fits these unit tests: callers specify only the API groups, objects, or interruption state they need. A Kubernetes integration fixture was considered, but would add setup cost without improving the targeted helper and output assertions.

Files changed (14) +930 / -286

Tests (14) +930 / -286
root_test.goName heap-dump method validation cases +14/-11

Name heap-dump method validation cases

• Runs each valid and invalid heap-dump method case as a named subtest for clearer failures.

internal/cli/root_test.go

clusterinfo_test.goCover cluster information output and interruption +98/-0

Cover cluster information output and interruption

• Tests the collector name, YAML file creation, namespace output, and behavior when collection is interrupted.

internal/collector/clusterinfo_test.go

collector_test.goCover collector configuration and registry +176/-0

Cover collector configuration and registry

• Adds cases for interruption, namespace selection, log time options, and expected registry entries.

internal/collector/collector_test.go

heapdump_test.goName instance matching and size formatting cases +23/-17

Name instance matching and size formatting cases

• Wraps existing table cases in descriptive subtests without changing their expectations.

internal/collector/heapdump_test.go

helm_test.goName secret-document filtering cases +14/-13

Name secret-document filtering cases

• Runs the existing Secret, ConfigMap, and Deployment filtering checks as named subtests.

internal/collector/helm_test.go

namespace_inspect_test.goName pattern-matching cases +12/-9

Name pattern-matching cases

• Adds descriptive subtests to the existing namespace-inspection pattern checks.

internal/collector/namespace_inspect_test.go

operator_test.goName operator relevance cases +17/-14

Name operator relevance cases

• Gives each existing RHDH-related name check its own descriptive subtest.

internal/collector/operator_test.go

output_test.goCover output helpers and resource resolution +185/-0

Cover output helpers and resource resolution

• Tests resource metadata and file output, known group-kind mappings, resource-name listing, and CRD type resolution using fake clients.

internal/collector/output_test.go

platform_test.goAdopt shared collector test configuration +18/-76

Adopt shared collector test configuration

• Switches platform cases to functional fixture options and removes the local fake-client and node factories.

internal/collector/platform_test.go

route_test.goCover route collection and nested field lookup +141/-0

Cover route collection and nested field lookup

• Tests route output when the API is absent, present with routes, or present without routes. Also covers nested string lookup.

internal/collector/route_test.go

testhelper_test.goCentralize fake collector client setup +96/-0

Centralize fake collector client setup

• Provides a shared test configuration factory with options for discovery groups, typed and dynamic objects, and interruption state. Moves the node factory out of platform tests.

internal/collector/testhelper_test.go

workload_test.goName workload owner-kind cases +10/-7

Name workload owner-kind cases

• Runs existing owner-reference kind mappings as named subtests.

internal/collector/workload_test.go

client_test.goConsolidate discovery fixtures and cases +58/-66

Consolidate discovery fixtures and cases

• Replaces two fake-client factories with one that accepts full group versions. Groups API-group and preferred-version checks under named subtests.

internal/kube/client_test.go

filter_test.goConsolidate namespace filtering tests +68/-73

Consolidate namespace filtering tests

• Reorganizes individual parsing, inclusion, and environment-wrapper tests into four table-driven tests with named cases.

internal/namespace/filter_test.go

@rhdh-qodo-merge

rhdh-qodo-merge Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Backstage test accepts unsupported version ✓ Resolved
Description
TestResolveCRDType mocks rhdh.redhat.com/v1alpha3 and checks the returned group and resource but
not its version, while rhdh-operator marks that Backstage version as unserved. The test can pass
with a version that would fail when describeCRD requests a Backstage resource from an
operator-managed cluster.
Code

internal/collector/output_test.go[150]

+		withAPIGroups("rhdh.redhat.com/v1alpha3", "sonataflow.org/v1alpha08"),
Relevance

●●● Strong

Accepted history favors correctness fixes preventing unsupported API behavior; asserting the served
version directly strengthens this test.

PR-#387
PR-#388

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new test supplies v1alpha3 and omits a version assertion; the operator CRD marks v1alpha3
unserved and v1alpha5 served.

rhdh-must-gather -> rhdh-operator
internal/collector/output_test.go[148-176]
internal/collector/output.go[111-151]
External repo: redhat-developer/rhdh-operator, config/crd/bases/rhdh.redhat.com_backstages.yaml [1149-1157]
External repo: redhat-developer/rhdh-operator, config/crd/bases/rhdh.redhat.com_backstages.yaml [2209-2215]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new Backstage resolution test uses v1alpha3, which rhdh-operator does not serve, and does not check the resolved version.
## Fix Focus Areas
- internal/collector/output_test.go[148-176]
- /cross_repos/rhdh-operator/config/crd/bases/rhdh.redhat.com_backstages.yaml[1149-1157]
## Recommended Fix
Use a version served by the operator, such as v1alpha5, in the fixture and assert that the resolved GVR has that version. Update the older kube test fixture for consistency.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 4 rules
✅ Cross-repo context — repo relationships
  Explored: repo: redhat-developer/rhdh-operator (sha: 0dea025a) — View relationship
Review mode: ⚖️ Balanced: Although test-only, this broad change adds substantial new test logic and shared fake-client infrastructure across multiple independent paths, warranting a complete review but not redundant extended passes.

Grey Divider

Tip of the day
💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread internal/collector/output_test.go Outdated
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

PR images are available (for 1 week):

  1. quay.io/rhdh-community/rhdh-must-gather:pr-421
  2. quay.io/rhdh-community/rhdh-must-gather:pr-421-94e4889ff

Use v1alpha5 (the currently served Backstage CRD version) instead
of v1alpha3 in TestResolveCRDType fixtures, and assert the resolved
GVR version field in both subtests to verify the full return value.

Assisted-by: Claude
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

PR images are available (for 1 week):

  1. quay.io/rhdh-community/rhdh-must-gather:pr-421
  2. quay.io/rhdh-community/rhdh-must-gather:pr-421-068456f27

@rm3l

rm3l commented Oct 6, 2026

Copy link
Copy Markdown
Member Author

/hold

Will use #435 to verify coverage diff upload in Codecov.

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Used by Prow. label Oct 6, 2026
@rm3l

rm3l commented Oct 6, 2026

Copy link
Copy Markdown
Member Author

/cherrypick release-2.1

@openshift-cherrypick-robot

Copy link
Copy Markdown

@rm3l: once the present PR merges, I will cherry-pick it on top of release-2.1 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-2.1

Instructions 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.

@rm3l rm3l changed the title test: improve Go test idioms across the codebase test: improve Go test idioms across the codebase [RHIDP-14400] Oct 6, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Used by Prow.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants