Repository navigation
NO-ISSUE: Add unit tests to validate table CEL expressions - #770
openshift-merge-bot[bot] merged 2 commits into
Conversation
|
@jhernand: This pull request explicitly references no jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (2)
WalkthroughThe PR converts reflection helpers to interfaces backed by unexported implementations, updates renderer, CLI, and terminal consumers to store helper values, adds generated mocks and renderer coverage, and removes HostClass table definitions. ChangesReflection helper migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)internal/cmd/cli/edit/edit_cmd.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.23][ERROR]: unable to find a config; path internal/cmd/cli/delete/delete_cmd.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.38][ERROR]: unable to find a config; path internal/cmd/cli/annotate/annotate_cmd.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.53][ERROR]: unable to find a config; path
Comment |
![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
🤖 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 `@internal/rendering/table_renderer_test.go`:
- Around line 67-70: The DELETING test setup is too broad because
helper.Lookup() always returns subnetHelper, which only stubs basic methods and
can break if unrelated lookup-backed columns are added. Update the test fixture
in table_renderer_test.go to use the same fallback lookup-helper pattern as the
CEL test, so only the DELETING behavior is exercised while any other lookup
columns get their own mocked helper. Keep the existing subnetHelper for the
intended DELETING path, but avoid routing every Lookup() call to it.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6fdab5aa-e4ff-43fc-8d21-d8dc75fb8e59
📒 Files selected for processing (18)
internal/cmd/cli/annotate/annotate_cmd.gointernal/cmd/cli/delete/delete_cmd.gointernal/cmd/cli/edit/edit_cmd.gointernal/cmd/cli/edit/edit_cmd_test.gointernal/cmd/cli/get/get_cmd.gointernal/cmd/cli/get/get_cmd_watch_e2e_test.gointernal/cmd/cli/label/label_cmd.gointernal/reflection/find_object.gointernal/reflection/reflection_helper.gointernal/reflection/reflection_helper_mock.gointernal/reflection/reflection_helper_test.gointernal/reflection/reflection_object_helper_mock.gointernal/rendering/table_renderer.gointernal/rendering/table_renderer_compute_instance_test.gointernal/rendering/table_renderer_test.gointernal/rendering/tables/osac.private.v1.HostClass.yamlinternal/rendering/tables/osac.public.v1.HostClass.yamlinternal/terminal/terminal_console.go
💤 Files with no reviewable changes (2)
- internal/rendering/tables/osac.public.v1.HostClass.yaml
- internal/rendering/tables/osac.private.v1.HostClass.yaml
249b97f to
50ee5a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/rendering/table_renderer_compute_instance_test.go`:
- Around line 47-49: The “Shows the template name” test case in
table_renderer_compute_instance_test.go is not validating the name path because
the template name, id, and lookup key are all the same value. Update the second
entry in the test setup to use a distinct template name from the id and spec
key, using the existing templateName argument and the ComputeInstanceTemplate
fields near templateObj so the assertion proves the renderer is displaying the
actual name rather than falling back to the key. This same adjustment should be
reflected in the duplicated test case referenced by the comment.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 64caa2a9-ee7e-4df9-bbe8-0bad14a74844
📒 Files selected for processing (4)
internal/rendering/table_renderer_compute_instance_test.gointernal/rendering/table_renderer_test.gointernal/rendering/tables/osac.private.v1.HostClass.yamlinternal/rendering/tables/osac.public.v1.HostClass.yaml
💤 Files with no reviewable changes (2)
- internal/rendering/tables/osac.public.v1.HostClass.yaml
- internal/rendering/tables/osac.private.v1.HostClass.yaml
50ee5a3 to
9d929e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/rendering/table_renderer_test.go`:
- Around line 148-156: The table rendering test can vacuously pass when the
tables directory is empty or contains no YAML files, so add a guard in
table_renderer_test.go around the existing loop that reads tableFiles and calls
Render to ensure at least one table definition was actually exercised. Use the
existing rendered counter in the test, increment it after each successful
Render, and assert after the loop that at least one table was rendered so the
“compile all tables” test fails when nothing was covered.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ee049f5a-1b28-4af3-83ce-00d2fc2a86ef
📒 Files selected for processing (4)
internal/rendering/table_renderer_compute_instance_test.gointernal/rendering/table_renderer_test.gointernal/rendering/tables/osac.private.v1.HostClass.yamlinternal/rendering/tables/osac.public.v1.HostClass.yaml
💤 Files with no reviewable changes (2)
- internal/rendering/tables/osac.private.v1.HostClass.yaml
- internal/rendering/tables/osac.public.v1.HostClass.yaml
|
💀 CI Triage: Root cause: A migration collision between the pre-built cluster snapshot and newly merged Helm chart changes causes strategic merge patch to fail during helm upgrade. Explanation: The failure is caused by a collision between the pre-built cluster snapshot (flavor) and the newly merged changes in osac-operator PR #316 and osac-installer PR #328. In the snapshot, the pre-installed 'osac-operator' deployment defines the 'OSAC_AAP_TOKEN' environment variable as a literal 'value'. However, the newly merged Helm chart changes configure 'OSAC_AAP_TOKEN' to use 'valueFrom.secretKeyRef'. When 'refresh-after-snapshot.py' runs 'helm upgrade osac', Kubernetes attempts to perform a strategic merge patch on the existing deployment. Because list elements are merged by name, the resulting environment variable has both 'value' and 'valueFrom' specified, which is invalid in Kubernetes and causes the upgrade to fail. Evidence: Suggestion: Rebuild the cluster snapshot (flavor) with the updated osac-operator deployment that uses valueFrom for OSAC_AAP_TOKEN. Alternatively, add a step in 'refresh-after-snapshot.py' to delete the existing 'osac-operator' deployment before running 'helm upgrade' so Helm can recreate it cleanly. Prow job | Build For deeper investigation, use the |
|
💀 CI Triage: Root cause: Helm upgrade fails during boot because the pre-installed osac-operator deployment has literal env vars that conflict with the new secretKeyRef configuration. Explanation: The failure is caused by a conflict during 'helm upgrade' in the boot step. A recent change in osac-operator (#316) and osac-installer (#328) updated the operator's Helm chart to source OSAC_AAP_TOKEN via secretKeyRef instead of a literal value. However, on snapshot-based clusters, the pre-installed osac-operator deployment already has OSAC_AAP_TOKEN and OSAC_AAP_URL injected as literal values. This causes a strategic-merge-patch conflict during 'helm upgrade', failing with 'Invalid value: "": may not be specified when value is not empty'. This broke the main branch for all E2E VMaaS jobs. The issue was resolved in osac-installer PR #333 (merged at 22:01 UTC), which strips these env vars before upgrading. Since this run started at 21:38 UTC, it did not include the fix. Retrying the job will resolve the failure. Evidence: Suggestion: /retest Prow job | Build For deeper investigation, use the |
|
/retest |
9d929e9 to
6ab8120
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@internal/rendering/table_renderer_compute_instance_test.go`:
- Around line 64-77: The test for the compute instance table renderer currently
allows any List options, so it won’t catch regressions in lookup filter
construction. Update the expectations around templateHelper.List in the compute
instance rendering test to inspect the passed reflection.ListOptions and assert
that options.Filter matches the expected lookupName predicate using this.id and
this.metadata.name for "my-template", while still returning the template
fixture.
In `@internal/rendering/table_renderer_test.go`:
- Around line 19-21: The table renderer test is globbing YAML files from the
working tree instead of validating the embedded tablesFS asset set used by
loadTable. Update the test setup in table_renderer_test.go to enumerate tablesFS
with fs.Glob (or equivalent fs-based traversal) and feed those paths into the
existing test cases, so the test exercises the same embedded assets production
loads; keep the current loadTable-focused assertions and adjust any
helpers/imports accordingly.
In `@internal/rendering/table_renderer.go`:
- Around line 83-91: The helper guard in TableRenderer can be bypassed by a
typed-nil reflection.Helper, so update SetHelper (or the Build nil check) to
normalize the incoming helper with reflection.NormalizeNil before storing or
validating it. Use the existing TableRenderer and Build methods to ensure the
mandatory-helper check treats typed-nil values as nil and still returns the
expected error when no real helper is set.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 78c34ab0-3afe-487f-adcc-773fae632b9b
📒 Files selected for processing (18)
internal/cmd/cli/annotate/annotate_cmd.gointernal/cmd/cli/delete/delete_cmd.gointernal/cmd/cli/edit/edit_cmd.gointernal/cmd/cli/edit/edit_cmd_test.gointernal/cmd/cli/get/get_cmd.gointernal/cmd/cli/get/get_cmd_watch_e2e_test.gointernal/cmd/cli/label/label_cmd.gointernal/reflection/find_object.gointernal/reflection/reflection_helper.gointernal/reflection/reflection_helper_mock.gointernal/reflection/reflection_helper_test.gointernal/reflection/reflection_object_helper_mock.gointernal/rendering/table_renderer.gointernal/rendering/table_renderer_compute_instance_test.gointernal/rendering/table_renderer_test.gointernal/rendering/tables/osac.private.v1.HostClass.yamlinternal/rendering/tables/osac.public.v1.HostClass.yamlinternal/terminal/terminal_console.go
💤 Files with no reviewable changes (2)
- internal/rendering/tables/osac.public.v1.HostClass.yaml
- internal/rendering/tables/osac.private.v1.HostClass.yaml
6ab8120 to
bcba64e
Compare
Replace the concrete `Helper` and `ObjectHelper` structs with interfaces, renaming the implementations to unexported `helper` and `objectHelper` types. Add `go:generate` directives for `mockgen` so that mocks are produced automatically. This is a preparation step for adding unit tests to the CLI rendering tables, which need to mock the reflection layer without requiring a live gRPC connection. Assisted-by: Cursor Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
Rewrite the table renderer tests to use mock reflection helpers instead of a real gRPC server, making them faster and independent of network infrastructure. Add a new test that scans all YAML table definition files, resolves the corresponding protobuf type from the registry, and renders an empty instance through the table renderer. This forces compilation and evaluation of every CEL expression, catching syntax errors and references to non-existent fields at test time. Remove the stale `HostClass` table definitions which referenced a type that no longer exists in the proto registry. Assisted-by: Cursor Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
bcba64e to
1dc5eed
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, jhernand 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 |
8eb016a
into
osac-project:main
4 participants
Summary
HelperandObjectHelperinterfaces from the reflection package with generated mocks,enabling tests to use mock helpers instead of live gRPC connections.
type, and renders an empty instance to force compilation of every CEL expression, catching syntax
errors and references to non-existent fields at test time.
server and improving test speed.
HostClasstable definitions that referenced types no longer in the proto registry.Test plan
ginkgo run internal/renderingpasses (6 specs)ginkgo run -r internalpassesSummary by CodeRabbit
Bug Fixes
Tests