NE-2450: Refactor query_prometheus to use DynamicClient and testify/suite#152
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bentito: This pull request references NE-2450 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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. |
|
@matzew I think this is ready to merge |
|
/assign @matzew |
|
@matzew I think ready for lgtm when you can |
|
I think the conflict here is perhaps due to this 4d7101f |
openshift-merge-robot
added
the
needs-rebase
bentito
force-pushed
the
refactor-query-prometheus
branch
from
d04ad2c to
fa2ec54
Compare
openshift-merge-robot
removed
the
needs-rebase
WalkthroughThe changes refactor the netedge toolset by renaming the query_prometheus.go package from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
♻️ Duplicate comments (1)
pkg/toolsets/netedge/coredns_test.go (1)
66-72:⚠️ Potential issue | 🟡 MinorCheck
AddToSchemeerror for test reliability.Line 68 ignores the error from
clientgoscheme.AddToScheme(scheme), butquery_prometheus_test.go(lines 95-96) properly checks this withs.Require().NoError(err). This inconsistency was flagged in a prior review and reportedly fixed, but the silent discard remains.Proposed fix
// Setup fake dynamic client scheme := runtime.NewScheme() - _ = clientgoscheme.AddToScheme(scheme) + err := clientgoscheme.AddToScheme(scheme) + s.Require().NoError(err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/toolsets/netedge/coredns_test.go` around lines 66 - 72, The test currently ignores the error returned by clientgoscheme.AddToScheme(scheme); change the call to capture the error and assert it succeeds (e.g., err := clientgoscheme.AddToScheme(scheme) followed by s.Require().NoError(err) or equivalent) so runtime.NewScheme()/clientgoscheme.AddToScheme are validated before creating fake.NewSimpleDynamicClient; update the block in coredns_test.go where scheme is created to mirror the pattern used in query_prometheus_test.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/toolsets/netedge/query_prometheus_test.go`:
- Around line 159-175: The error-checking branch for tt.expectError is inverted:
when expecting an error you should accept either a returned error (err != nil)
or a non-nil result with result.Error set, and you must not call s.NoError(err)
when err is non-nil. Change the block handling tt.expectError so it asserts that
either err is non-nil (use s.Error(err) or s.NotNil(err)) OR (result != nil &&
result.Error != nil); if both are nil then call s.Fail("expected error but got
nil"); ensure the symbols referenced are tt.expectError, err, result and
result.Error so the intent is clear and tests only fail when neither source
reports an error.
---
Duplicate comments:
In `@pkg/toolsets/netedge/coredns_test.go`:
- Around line 66-72: The test currently ignores the error returned by
clientgoscheme.AddToScheme(scheme); change the call to capture the error and
assert it succeeds (e.g., err := clientgoscheme.AddToScheme(scheme) followed by
s.Require().NoError(err) or equivalent) so
runtime.NewScheme()/clientgoscheme.AddToScheme are validated before creating
fake.NewSimpleDynamicClient; update the block in coredns_test.go where scheme is
created to mirror the pattern used in query_prometheus_test.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aacff6c0-e334-4f59-8d0a-a5c1578108f4
📒 Files selected for processing (5)
pkg/toolsets/netedge/coredns.gopkg/toolsets/netedge/coredns_test.gopkg/toolsets/netedge/query_prometheus.gopkg/toolsets/netedge/query_prometheus_test.gopkg/toolsets/netedge/toolset.go
| if tt.expectError { | ||
| if err == nil { | ||
| // Check if result has Error field (ToolCallResult) | ||
| if result == nil || result.Error == nil { | ||
| t.Errorf("expected error but got nil") | ||
| s.Fail("expected error but got nil") | ||
| } | ||
| } else { | ||
| // Logic error in test assumption: handler returns (result, nil) usually | ||
| // but check if err is returned | ||
| assert.NoError(t, err) | ||
| s.NoError(err) | ||
| } | ||
| } else { | ||
| assert.NoError(t, err) | ||
| if assert.NotNil(t, result) { | ||
| assert.NoError(t, result.Error) | ||
| s.NoError(err) | ||
| if s.NotNil(result) { | ||
| s.NoError(result.Error) | ||
| for _, expected := range tt.expectedContains { | ||
| assert.Contains(t, result.Content, expected) | ||
| s.Contains(result.Content, expected) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Confusing error validation logic.
The error handling branch at lines 164-166 calls s.NoError(err) when err != nil, which will always fail if reached. The intent appears to be accepting either form of error (return error or result.Error), but the current logic is inverted.
Proposed fix to clarify the intent
// Validation
if tt.expectError {
- if err == nil {
- if result == nil || result.Error == nil {
- s.Fail("expected error but got nil")
- }
- } else {
- s.NoError(err)
- }
+ // Handler may return error via return value OR via result.Error
+ hasError := err != nil || (result != nil && result.Error != nil)
+ s.True(hasError, "expected error but got none")
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/toolsets/netedge/query_prometheus_test.go` around lines 159 - 175, The
error-checking branch for tt.expectError is inverted: when expecting an error
you should accept either a returned error (err != nil) or a non-nil result with
result.Error set, and you must not call s.NoError(err) when err is non-nil.
Change the block handling tt.expectError so it asserts that either err is
non-nil (use s.Error(err) or s.NotNil(err)) OR (result != nil && result.Error !=
nil); if both are nil then call s.Fail("expected error but got nil"); ensure the
symbols referenced are tt.expectError, err, result and result.Error so the
intent is clear and tests only fail when neither source reports an error.
This refactors query_prometheus to use DynamicClient and NetEdgeTestSuite, aligning with PR openshift#151 and PR openshift#144. Moves query_prometheus to the netedge package to share test infrastructure. Signed-off-by: bentito <btofel@redhat.com>
Ensure clientgoscheme.AddToScheme does not return an error in test setup. Signed-off-by: bentito <btofel@redhat.com>
matzew
force-pushed
the
refactor-query-prometheus
branch
from
fa2ec54 to
3e410fe
Compare
|
/override "Konflux kflux-prd-rh02 / openshift-mcp-server-on-pull-request" |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/toolsets/netedge/query_prometheus_test.go (1)
159-166:⚠️ Potential issue | 🟡 MinorInverted error validation logic.
The error handling at lines 164-165 calls
s.NoError(err)whenerr != nil, which will always fail if reached. The intent is to accept either an error via return value or viaresult.Error, but the current logic is inverted.Proposed fix
// Validation if tt.expectError { - if err == nil { - if result == nil || result.Error == nil { - s.Fail("expected error but got nil") - } - } else { - s.NoError(err) - } + // Handler may return error via return value OR via result.Error + hasError := err != nil || (result != nil && result.Error != nil) + s.True(hasError, "expected error but got none") } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/toolsets/netedge/query_prometheus_test.go` around lines 159 - 166, The test's error-assertion is inverted: when tt.expectError is true you must accept either a non-nil err OR a non-nil result.Error, not call s.NoError on a real err; update the block around tt.expectError to: if tt.expectError { if err == nil { if result == nil || result.Error == nil { s.Fail("expected error but got nil") } } /* else: err != nil -> acceptable, do nothing */ } else { s.NoError(err) } — locate and change the logic referencing tt.expectError, err, result.Error, s.Fail and s.NoError in the test function to implement this control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/toolsets/netedge/query_prometheus_test.go`:
- Around line 159-166: The test's error-assertion is inverted: when
tt.expectError is true you must accept either a non-nil err OR a non-nil
result.Error, not call s.NoError on a real err; update the block around
tt.expectError to: if tt.expectError { if err == nil { if result == nil ||
result.Error == nil { s.Fail("expected error but got nil") } } /* else: err !=
nil -> acceptable, do nothing */ } else { s.NoError(err) } — locate and change
the logic referencing tt.expectError, err, result.Error, s.Fail and s.NoError in
the test function to implement this control flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 343d107a-60cf-4cb0-b398-82c89b5ed89c
📒 Files selected for processing (3)
pkg/toolsets/netedge/query_prometheus.gopkg/toolsets/netedge/query_prometheus_test.gopkg/toolsets/netedge/toolset.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/toolsets/netedge/query_prometheus.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bentito, matzew 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 |
openshift-ci
bot
added
the
approved
|
@matzew: Overrode contexts on behalf of matzew: Konflux kflux-prd-rh02 / openshift-mcp-server-on-pull-request 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 kubernetes-sigs/prow repository. |
|
@bentito: 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. |
openshift-merge-bot
bot
merged commit f419a94
into
openshift:main
4 participants
This PR refactors the
query_prometheustool to useDynamicClientand migrate its tests to useNetEdgeTestSuite, moving it to thenetedgepackage to share test infrastructure.This builds upon the following work:
get_coredns_config#131 (Initial introduction ofget_coredns_config)get_coredns_config)get_service_endpoints)The testing and patterns are now unified.