Add sosreport tools e2e test#16
Conversation
Meina-rh
force-pushed
the
sosreport_e2e
branch
from
3de4e48 to
31f8f45
Compare
Meina-rh
force-pushed
the
sosreport_e2e
branch
from
31f8f45 to
8699b1b
Compare
|
@coderabbitai : please review |
|
@tssurya: I'll review the changes in this PR for you. ✅ Actions performedReview triggered.
|
|
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:
📝 WalkthroughWalkthroughAdds an offline-mode sosreport end-to-end test and propagates an MCP_MODE flag through CI, Makefile, run script, and test bootstrap. CI gains a matrix for live-cluster vs offline with conditional KIND and log steps; run-e2e and e2e suite branch behavior based on MCP_MODE. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Runner as Job Runner
participant KIND as KIND Cluster
participant Script as run-e2e.sh
participant Tests as Ginkgo E2E Tests
participant MCP as MCPInspector / Testdata
GH->>Runner: start job (matrix.mode)
alt mode == live-cluster
Runner->>KIND: deploy KIND cluster
end
Runner->>Script: invoke ./hack/run-e2e.sh (MCP_MODE)
Script->>Tests: run ginkgo with MCP_MODE
alt MCP_MODE == offline
Tests->>MCP: initialize offline MCPInspector using testdata sosreport
Tests->>MCP: run plugin/command/list/search/get and pod-log queries
else MCP_MODE == live-cluster
Tests->>KIND: use kubeconfig to interact with cluster
Tests->>MCP: query live MCP resources
end
alt mode == live-cluster
Runner->>GH: upload KIND logs and cleanup
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 0
🧹 Nitpick comments (2)
test/e2e/sosreport_test.go (2)
30-32: Consider usingruntime.Callerfor more robust testdata path resolution.The relative path
../../pkg/sosreport/mcp/testdata/sosreportresolves against the current working directory. Whilego testtypically runs from the package directory making this work, it can break if tests are invoked from a different directory (e.g., repo root with-v ./...).🔎 Alternative approach using runtime.Caller:
+import "runtime" + +func getTestdataPath() string { + _, filename, _, _ := runtime.Caller(0) + return filepath.Join(filepath.Dir(filename), "../../pkg/sosreport/mcp/testdata/sosreport") +}Then in BeforeEach:
- testdataSosreportPath, err = filepath.Abs("../../pkg/sosreport/mcp/testdata/sosreport") - Expect(err).NotTo(HaveOccurred()) + testdataSosreportPath = getTestdataPath()
198-203: Clarify intent of pattern filter validation.This assertion verifies that every line in the output either contains "eth0" or is empty, which validates strict pattern-only filtering. However, the nested condition logic is slightly convoluted.
🔎 Consider a clearer assertion:
- // Verify lo interface is not in filtered output - lines := strings.Split(strings.TrimSpace(result.Output), "\n") - for _, line := range lines { - if !strings.Contains(line, "eth0") { - Expect(line).To(BeEmpty()) - } - } + // Verify all non-empty lines contain the filter pattern + lines := strings.Split(strings.TrimSpace(result.Output), "\n") + for _, line := range lines { + if line != "" { + Expect(line).To(ContainSubstring("eth0"), "Expected filtered output to only contain lines matching 'eth0'") + } + }Or more concisely with a negative assertion:
// Verify unrelated interfaces are not in filtered output Expect(result.Output).NotTo(ContainSubstring("lo:"))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/e2e/sosreport_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/sosreport_test.go (4)
test/e2e/inspector/mcp_inspector.go (2)
MCPInspector(15-21)NewMCPInspector(23-25)test/e2e/utils/utils.go (1)
UnmarshalCallToolResult(11-25)pkg/sosreport/types/command.go (3)
ListPluginsResult(56-59)ListCommandsResult(10-14)GetCommandResult(51-53)pkg/sosreport/types/logs.go (1)
SearchPodLogsResult(12-14)
🔇 Additional comments (4)
test/e2e/sosreport_test.go (4)
42-68: Well-structured test with clear assertions.Good use of
Bysteps for documentation and the map-based plugin validation approach makes the assertions readable and maintainable.
70-115: LGTM!Both test cases follow a consistent pattern. The
HaveLenassertion before direct index access on line 113 ensures safety.
227-281: Good test coverage for pod log search functionality.The three test cases cover key scenarios: pattern matching, pod filtering, and specific log content search. The assertions appropriately validate expected output content.
117-139: No issues found. TheSearchCommandsResulttype is properly defined inpkg/sosreport/types/command.gowith all required fields (TotalandMatches), and theCommandMatchstruct has the expected fields (Plugin,Exec,Filepath) that are correctly used in the test assertions.
Meina-rh
force-pushed
the
sosreport_e2e
branch
from
8699b1b to
8d6db36
Compare
|
@Meina-rh I think it'd be better to create a separate e2e test suite for offline mode. Otherwise we'll just end up creating the kind cluster but will never use it. PLMK if you need help in how to go about it. |
|
@arkadeepsen I've added |
@Meina-rh even if you use use |
Meina-rh
force-pushed
the
sosreport_e2e
branch
from
d4a2fc5 to
4f60062
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/e2e.yml:
- Around line 40-51: The workflow step "Install dependencies" writes the local
bin path to GITHUB_PATH without quoting which can cause word-splitting/globbing;
update the echo that appends the path so it quotes the variable expansion (use
"$GITHUB_PATH" when redirecting) to ensure the entire path is written safely and
robustly; locate the echo line that currently reads echo "$HOME/.local/bin" >>
$GITHUB_PATH and change the redirection target to the quoted variable name.
4f60062 to
332b9a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/e2e/e2e_suite_test.go`:
- Around line 94-110: Read kubeconfig from the KUBECONFIG env var or fall back
to k8se2eframework.TestContext.KubeConfig, always create mcpInspector via
inspector.NewMCPInspector().Command(mcpServerPath) and append the kubeconfig
CommandFlags only if a kubeconfig value exists; only attempt to build client
config and set kubeClient (using client.New) when a kubeconfig value is
available, and ensure k8se2eframework.TestContext.KubeConfig is populated from
the resolved kubeconfig variable so tests that rely on the flag path work.
🧹 Nitpick comments (1)
hack/run-e2e.sh (1)
44-49: Normalize MCP_MODE to handle variant formats safely.Currently the script only matches the exact string "Offline Mode", but callers could pass "offline-mode" (from matrix definitions) or other casings, causing the condition to silently fall through to online tests. Normalizing once prevents this failure mode.
♻️ Suggested change
+MCP_MODE_NORMALIZED=$(echo "${MCP_MODE}" | tr '[:upper:]' '[:lower:]' | tr ' ' '-') -if [[ "${MCP_MODE}" == "Offline Mode" ]]; then +if [[ "${MCP_MODE_NORMALIZED}" == "offline-mode" ]]; then echo "Running offline mode tests (sosreport and must-gather)" ginkgo -vv --focus="Sosreport Tools" test/e2e else echo "Running online mode tests (excluding offline tests)" ginkgo -vv --skip="Sosreport Tools" test/e2e fi
test/e2e/e2e_suite_test.go
Outdated
| kubeconfig := os.Getenv(kubeconfigEnvVar) | ||
| // Only setup kubeClient and mcpInspector if KUBECONFIG is provided | ||
| // This allows offline mode e2e tests to run without a cluster | ||
| if kubeconfig != "" { | ||
| config, err := clientcmd.BuildConfigFromFlags("", kubeconfig) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| kubeClient, err = client.New(config, client.Options{Scheme: s}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| k8se2eframework.TestContext.KubeConfig = kubeconfig | ||
|
|
||
| mcpInspector = inspector.NewMCPInspector(). | ||
| Command(mcpServerPath). | ||
| CommandFlags(map[string]string{ | ||
| "kubeconfig": kubeconfig, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the exact lines 94-110 in the file mentioned
cat -n test/e2e/e2e_suite_test.go | sed -n '90,115p'Repository: ovn-kubernetes/ovn-kubernetes-mcp
Length of output: 1009
🏁 Script executed:
# Search for all mcpInspector usages and their context
rg -n --type=go -B3 -A3 'mcpInspector'Repository: ovn-kubernetes/ovn-kubernetes-mcp
Length of output: 3397
🏁 Script executed:
# Search for TestContext.KubeConfig usages
rg -n --type=go -B2 -A2 'TestContext\.KubeConfig'Repository: ovn-kubernetes/ovn-kubernetes-mcp
Length of output: 2831
🏁 Script executed:
# Search for KUBECONFIG handling
rg -n --type=go -B2 -A2 'kubeconfigEnvVar|KUBECONFIG'Repository: ovn-kubernetes/ovn-kubernetes-mcp
Length of output: 2973
Always initialize mcpInspector and fall back to flag-based kubeconfig.
Line 94 reads kubeconfig only from the KUBECONFIG env var, ignoring the --kubeconfig command-line flag (available via TestContext.KubeConfig). More critically, mcpInspector is left nil if the env var is empty, but tests in ovn_test.go (lines 55, 82) and kubernetes_test.go (lines 41, 89, 133) call mcpInspector.MethodCall() unconditionally without nil checks, risking nil dereference. Restructure to fall back to TestContext.KubeConfig when the env var is unset, and always create the inspector, only adding the kubeconfig flag when present.
🐛 Suggested fix
- kubeconfig := os.Getenv(kubeconfigEnvVar)
+ kubeconfig := os.Getenv(kubeconfigEnvVar)
+ if kubeconfig == "" {
+ kubeconfig = k8se2eframework.TestContext.KubeConfig
+ }
// Only setup kubeClient and mcpInspector if KUBECONFIG is provided
// This allows offline mode e2e tests to run without a cluster
if kubeconfig != "" {
config, err := clientcmd.BuildConfigFromFlags("", kubeconfig)
Expect(err).NotTo(HaveOccurred())
kubeClient, err = client.New(config, client.Options{Scheme: s})
Expect(err).NotTo(HaveOccurred())
k8se2eframework.TestContext.KubeConfig = kubeconfig
-
- mcpInspector = inspector.NewMCPInspector().
- Command(mcpServerPath).
- CommandFlags(map[string]string{
- "kubeconfig": kubeconfig,
- })
}
+ mcpInspector = inspector.NewMCPInspector().Command(mcpServerPath)
+ if kubeconfig != "" {
+ mcpInspector.CommandFlags(map[string]string{
+ "kubeconfig": kubeconfig,
+ })
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kubeconfig := os.Getenv(kubeconfigEnvVar) | |
| // Only setup kubeClient and mcpInspector if KUBECONFIG is provided | |
| // This allows offline mode e2e tests to run without a cluster | |
| if kubeconfig != "" { | |
| config, err := clientcmd.BuildConfigFromFlags("", kubeconfig) | |
| Expect(err).NotTo(HaveOccurred()) | |
| kubeClient, err = client.New(config, client.Options{Scheme: s}) | |
| Expect(err).NotTo(HaveOccurred()) | |
| k8se2eframework.TestContext.KubeConfig = kubeconfig | |
| mcpInspector = inspector.NewMCPInspector(). | |
| Command(mcpServerPath). | |
| CommandFlags(map[string]string{ | |
| "kubeconfig": kubeconfig, | |
| }) | |
| } | |
| kubeconfig := os.Getenv(kubeconfigEnvVar) | |
| if kubeconfig == "" { | |
| kubeconfig = k8se2eframework.TestContext.KubeConfig | |
| } | |
| // Only setup kubeClient and mcpInspector if KUBECONFIG is provided | |
| // This allows offline mode e2e tests to run without a cluster | |
| if kubeconfig != "" { | |
| config, err := clientcmd.BuildConfigFromFlags("", kubeconfig) | |
| Expect(err).NotTo(HaveOccurred()) | |
| kubeClient, err = client.New(config, client.Options{Scheme: s}) | |
| Expect(err).NotTo(HaveOccurred()) | |
| k8se2eframework.TestContext.KubeConfig = kubeconfig | |
| } | |
| mcpInspector = inspector.NewMCPInspector().Command(mcpServerPath) | |
| if kubeconfig != "" { | |
| mcpInspector.CommandFlags(map[string]string{ | |
| "kubeconfig": kubeconfig, | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/e2e_suite_test.go` around lines 94 - 110, Read kubeconfig from the
KUBECONFIG env var or fall back to k8se2eframework.TestContext.KubeConfig,
always create mcpInspector via
inspector.NewMCPInspector().Command(mcpServerPath) and append the kubeconfig
CommandFlags only if a kubeconfig value exists; only attempt to build client
config and set kubeClient (using client.New) when a kubeconfig value is
available, and ensure k8se2eframework.TestContext.KubeConfig is populated from
the resolved kubeconfig variable so tests that rely on the flag path work.
332b9a6 to
20fa0db
Compare
|
@arkadeepsen I try to create a separate test suite for offline mode following by jamo's PR. is this what you mean? |
Yes somewhat similar to this. I'll take a look at this. But since you are using Jamo's commit, it would be better we first merge his PR first. |
Meina-rh
force-pushed
the
sosreport_e2e
branch
2 times, most recently
from
0d71e54 to
339eaf8
Compare
arkadeepsen
left a comment
arkadeepsen
left a comment
There was a problem hiding this comment.
Let's separate out the addition of the new CI lane into another commit. I'll review the actual e2e test post that.
test/e2e/e2e_suite_test.go
Outdated
| mcpMode := os.Getenv(mcpModeEnvVar) | ||
| // Skip kubeconfig setup for offline mode tests | ||
| // Offline tests (sosreport, must-gather) don't need a running cluster | ||
| if mcpMode == "Offline Mode" { |
There was a problem hiding this comment.
Let's introduce a const with the value offline instead of directly matching with string.
hack/run-e2e.sh
Outdated
| ginkgo -vv --focus="Sosreport Tools" test/e2e | ||
| else | ||
| echo "Running online mode tests (excluding offline tests)" | ||
| ginkgo -vv --skip="Sosreport Tools" test/e2e |
There was a problem hiding this comment.
Can this be made more generic? If we add more layers to the offline mode, then we'd have to update this everytime.
There was a problem hiding this comment.
I update with [offline] label in sosreport Describe and filter this label
Meina-rh
force-pushed
the
sosreport_e2e
branch
6 times, most recently
from
0d085a6 to
ec2faf8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/e2e/sosreport_test.go`:
- Around line 16-33: The test currently computes testdataSosreportPath with
filepath.Abs("../../pkg/sosreport/mcp/testdata/sosreport") which depends on the
working directory; update the BeforeEach in this test to derive the
repository-relative path using the file location: call runtime.Caller(0) inside
the BeforeEach to get this test file's directory (use filepath.Dir on the
returned filename) and then filepath.Join that directory with the relative path
" ../../pkg/sosreport/mcp/testdata/sosreport" (or better, the path segments
"pkg","sosreport","mcp","testdata","sosreport") to produce
testdataSosreportPath; keep the variable name testdataSosreportPath and the
Expect(err).NotTo(HaveOccurred()) check for any errors from runtime.Caller or
path operations.
arkadeepsen
left a comment
arkadeepsen
left a comment
There was a problem hiding this comment.
@Meina-rh Great effort to add the 2 CI lanes and sosreport e2e tests. Let's add some failure test scenarios as well where some of the commands are expected to fail.
test/e2e/sosreport_test.go
Outdated
| var sosreportInspector *inspector.MCPInspector | ||
| var testdataSosreportPath string | ||
|
|
||
| BeforeEach(func() { | ||
| mcpServerPath := os.Getenv(mcpServerPathEnvVar) | ||
| Expect(mcpServerPath).NotTo(BeEmpty()) | ||
|
|
||
| sosreportInspector = inspector.NewMCPInspector(). | ||
| Command(mcpServerPath). | ||
| CommandFlags(map[string]string{ | ||
| "mode": "offline", | ||
| }) |
There was a problem hiding this comment.
I think we should reuse the same mcpInspector global variable instead of introducing sosreportInspector. Since we are not going to run live-cluster and offline tests together, it shouldn't be a problem. No need to intialize the mcpServerPath and mcpInspector everytime before run each test.
There was a problem hiding this comment.
yep, I update mcpInpsector for offline mode in e2e_suite_test.go.
test/e2e/sosreport_test.go
Outdated
| Expect(result.Plugin).To(Equal("networking")) | ||
| Expect(result.CommandCount).To(Equal(1)) | ||
| Expect(result.Commands).To(HaveLen(1)) | ||
| Expect(result.Commands[0].Exec).To(Equal("ip addr show")) |
There was a problem hiding this comment.
We can add the command path check for networking plugin as well.
There was a problem hiding this comment.
ok. I also update some tests using ginkgo.DescribeTable
test/e2e/sosreport_test.go
Outdated
| // Verify output contains expected content | ||
| Expect(result.Output).To(ContainSubstring("Bridge br-int")) | ||
| Expect(result.Output).To(ContainSubstring("ovn-k8s-mp0")) | ||
| Expect(result.Output).To(ContainSubstring("ovs_version: \"2.17.0\"")) |
There was a problem hiding this comment.
We can probably remove the ovs version check. This is very specific to the testdata and may cause an issue when the testdata is updated in the future.
test/e2e/sosreport_test.go
Outdated
| lines := strings.Split(strings.TrimSpace(result.Output), "\n") | ||
| for _, line := range lines { | ||
| if !strings.Contains(line, "eth0") { | ||
| Expect(line).To(BeEmpty()) | ||
| } | ||
| } |
There was a problem hiding this comment.
| lines := strings.Split(strings.TrimSpace(result.Output), "\n") | |
| for _, line := range lines { | |
| if !strings.Contains(line, "eth0") { | |
| Expect(line).To(BeEmpty()) | |
| } | |
| } | |
| iter := strings.SplitSeq(strings.TrimSpace(result.Output), "\n") | |
| for line := range iter { | |
| if !strings.Contains(line, "eth0") { | |
| Expect(line).To(BeEmpty()) | |
| } | |
| } |
SplitSeq can be used here instead since we are not using the split slice here.
Additionally, shouldn't the check be that if eth0 is not contained in a line, the test fails? Meaning each line should consist of eth0.
| .PHONY: run-e2e | ||
| run-e2e: | ||
| ./hack/run-e2e.sh $(NVM_VERSION) $(NODE_VERSION) $(NPM_VERSION) | ||
| ./hack/run-e2e.sh $(NVM_VERSION) $(NODE_VERSION) $(NPM_VERSION) "$(MCP_MODE)" |
There was a problem hiding this comment.
Let's add a default value for MCP_MODE here in the makefile. I think default value should be live-cluster here so that the current behavior doesn't break.
There was a problem hiding this comment.
Also add similar conditional kind deployment for test-e2e make target, like you've done in the e2e.yml file.
Meina-rh
force-pushed
the
sosreport_e2e
branch
from
ec2faf8 to
f489908
Compare
Makefile
Outdated
| ifeq ($(MCP_MODE),live-cluster) | ||
| $(MAKE) deploy-kind-ovnk | ||
| endif | ||
| $(MAKE) run-e2e || EXIT_CODE=$$?; \ | ||
| ifeq ($(MCP_MODE),live-cluster) | ||
| $(MAKE) undeploy-kind-ovnk; \ | ||
| endif |
There was a problem hiding this comment.
| ifeq ($(MCP_MODE),live-cluster) | |
| $(MAKE) deploy-kind-ovnk | |
| endif | |
| $(MAKE) run-e2e || EXIT_CODE=$$?; \ | |
| ifeq ($(MCP_MODE),live-cluster) | |
| $(MAKE) undeploy-kind-ovnk; \ | |
| endif | |
| if [ "$(MCP_MODE)" = "live-cluster" ]; then $(MAKE) deploy-kind-ovnk; fi; \ | |
| $(MAKE) run-e2e || EXIT_CODE=$$?; \ | |
| if [ "$(MCP_MODE)" = "live-cluster" ]; then $(MAKE) undeploy-kind-ovnk; fi; \ |
Ran it locally and the second ifeq wasn't working as expected. Better to use shell if here.
Meina-rh
force-pushed
the
sosreport_e2e
branch
6 times, most recently
from
e7dadbb to
6bc6027
Compare
Signed-off-by: Meina-rh <meinli@redhat.com>
Meina-rh
force-pushed
the
sosreport_e2e
branch
from
6bc6027 to
004267f
Compare
|
@arkadeepsen I added some failed tests. And I found a issue on
Here is fix commit: 004267f. PTAL, thanks |
.github/workflows/e2e.yml
Outdated
| - {mode: "live-cluster"} | ||
| - {mode: "offline"} | ||
| env: | ||
| JOB_NAME: "${{ matrix.mode }}" |
There was a problem hiding this comment.
This env var is not being used.
There was a problem hiding this comment.
does github CI job name use this env var?
There was a problem hiding this comment.
So let's use it if we are keeping it. We can use it here to name the artifacts uploaded: https://github.com/ovn-kubernetes/ovn-kubernetes-mcp/pull/16/changes#diff-3e103440521ada06efd263ae09b259e5507e4b8f7408308dc227621ad9efa31eR92
- name: kind-logs-e2e-${{ github.run_id }}
+ name: kind-logs-e2e-${{ matrix.mode }}-${{ github.run_id }}
test/e2e/sosreport_test.go
Outdated
|
|
||
| By("Checking the result") | ||
| result := utils.UnmarshalCallToolResult[sostypes.ListPluginsResult](output) | ||
| Expect(result.Plugins).To(HaveLen(expectedPluginCount)) |
There was a problem hiding this comment.
Why is only plugin count parameterized but the other values are hardoced in the test? Best to follow one approach rather than mixing them.
Signed-off-by: Meina-rh <meinli@redhat.com>
Meina-rh
force-pushed
the
sosreport_e2e
branch
from
004267f to
2925a31
Compare
when output is empty, the Matches slice in SearchCommandsResult was nil. When marshaled and returned via MCP, this resulted in "matches": null in the JSON output, causing schema validation failures. This fix ensures that even when no commands are found, the JSON output always contains "matches": [] and conforms to the MCP tool schema. Signed-off-by: Meina-rh <meinli@redhat.com>
Meina-rh
force-pushed
the
sosreport_e2e
branch
from
2925a31 to
bb596cd
Compare
3 participants
Add sosreport tools e2e test
Summary by CodeRabbit
Tests
Chores