Skip to content

NE-2450: Refactor query_prometheus to use DynamicClient and testify/suite #152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 2 commits into from
Mar 16, 2026
Merged

NE-2450: Refactor query_prometheus to use DynamicClient and testify/suite #152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2ee9f7
Refactor query_prometheus (NE-2450): Use DynamicClient & testify/suite
bentito Feb 19, 2026
3e410fe
tests(prometheus): check AddToScheme error
bentito Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...oolsets/netedge/tools/query_prometheus.go → pkg/toolsets/netedge/query_prometheus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package tools
package netedge

import (
"context"
Expand Down
95 changes: 20 additions & 75 deletions ...ts/netedge/tools/query_prometheus_test.go → ...toolsets/netedge/query_prometheus_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,64 +1,21 @@
package tools
package netedge

import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"testing"
"time"

"github.com/containers/kubernetes-mcp-server/pkg/api"
"github.com/containers/kubernetes-mcp-server/pkg/prometheus"
"github.com/stretchr/testify/assert"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/dynamic"
fakedynamic "k8s.io/client-go/dynamic/fake"
"k8s.io/client-go/rest"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
)

// MockConfigProvider partially implements api.BaseConfig
type MockConfigProvider struct {
api.BaseConfig // Embed to satisfy interface
}

func (m *MockConfigProvider) GetToolsetConfig(name string) (api.ExtendedConfig, bool) {
return nil, false
}

// MockKubernetesClient implements api.KubernetesClient with DynamicClient support
type MockKubernetesClient struct {
api.KubernetesClient // Embed to satisfy interface
Token string
DynClient dynamic.Interface
}

func (m *MockKubernetesClient) RESTConfig() *rest.Config {
return &rest.Config{
BearerToken: m.Token,
TLSClientConfig: rest.TLSClientConfig{
Insecure: true,
},
}
}

func (m *MockKubernetesClient) DynamicClient() dynamic.Interface {
return m.DynClient
}

// MockToolCallRequest implements api.ToolCallRequest
type MockToolCallRequest struct {
Args map[string]any
}

func (m *MockToolCallRequest) GetArguments() map[string]any {
return m.Args
}

func TestQueryPrometheusHandler_Diagnostics(t *testing.T) {
func (s *NetEdgeTestSuite) TestQueryPrometheusHandler_Diagnostics() {
// 1. Setup mock Prometheus server (TLS)
ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Verify Authentication
Expand Down Expand Up @@ -135,6 +92,8 @@ func TestQueryPrometheusHandler_Diagnostics(t *testing.T) {
}

scheme := runtime.NewScheme()
err := clientgoscheme.AddToScheme(scheme)
s.Require().NoError(err)
dynClient := fakedynamic.NewSimpleDynamicClient(scheme, route)

tests := []struct {
Expand Down Expand Up @@ -184,47 +143,33 @@ func TestQueryPrometheusHandler_Diagnostics(t *testing.T) {
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Setup dependencies
cfgProvider := &MockConfigProvider{}
kubeClient := &MockKubernetesClient{
Token: "fake-token",
DynClient: dynClient,
}
toolReq := &MockToolCallRequest{
Args: map[string]any{
"diagnostic_target": tt.diagTarget,
},
}
s.Run(tt.name, func() {
// Ensure Insecure is true for prometheus insecure test handling
s.mockClient.restConfig.BearerToken = "fake-token"
s.mockClient.restConfig.Insecure = true

// Construct params
params := api.ToolHandlerParams{
Context: context.Background(),
BaseConfig: cfgProvider,
KubernetesClient: kubeClient,
ToolCallRequest: toolReq,
}
s.SetDynamicClient(dynClient)
s.SetArgs(map[string]any{
"diagnostic_target": tt.diagTarget,
})

result, err := queryPrometheusHandler(params)
result, err := queryPrometheusHandler(s.params)

// Validation
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)
}
}
}
Comment on lines 159 to 175
Copy link

@coderabbitai coderabbitai bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Expand Down
3 changes: 1 addition & 2 deletions pkg/toolsets/netedge/toolset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"github.com/containers/kubernetes-mcp-server/pkg/api"
"github.com/containers/kubernetes-mcp-server/pkg/toolsets"
"github.com/containers/kubernetes-mcp-server/pkg/toolsets/netedge/internal/defaults"
netedgeTools "github.com/containers/kubernetes-mcp-server/pkg/toolsets/netedge/tools"
)

// Toolset implements the netedge toolset for Network Ingress & DNS troubleshooting.
Expand All @@ -24,7 +23,7 @@ func (t *Toolset) GetDescription() string {

func (t *Toolset) GetTools(_ api.Openshift) []api.ServerTool {
return slices.Concat(
netedgeTools.InitQueryPrometheus(),
InitQueryPrometheus(),
initCoreDNS(),
initEndpoints(),
)
Expand Down