ctl: add unit tests for all kmeshctl subcommands#1599
ctl: add unit tests for all kmeshctl subcommands#1599Eeshu-Yadav wants to merge 2 commits intokmesh-net:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive suite of unit tests for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Code is written, then Tests are added, bugs flee fast Software now shines bright. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a good set of initial unit tests for the kmeshctl subcommands, focusing on the Cobra command structure and helper functions. However, a significant improvement would be to also test the execution logic within the Run functions for commands in packages like authz, dump, and monitoring. Currently, this core logic is untested. The tests for the log subcommand serve as an excellent template, using httptest to mock the backend and test the command's behavior end-to-end without a real Kubernetes cluster. Adopting this pattern across the other subcommands would greatly increase test coverage and the overall quality of the CLI tool.
| func TestNewCmd(t *testing.T) { | ||
| cmd := NewCmd() | ||
| if cmd.Use != "authz" { | ||
| t.Fatalf("Use = %q, want %q", cmd.Use, "authz") | ||
| } | ||
|
|
||
| got := map[string]bool{} | ||
| for _, sub := range cmd.Commands() { | ||
| got[sub.Name()] = true | ||
| } | ||
| for _, want := range []string{"enable", "disable", "status"} { | ||
| if !got[want] { | ||
| t.Errorf("subcommand %q not registered", want) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestNewEnableCmd(t *testing.T) { | ||
| cmd := NewEnableCmd() | ||
| if cmd.Use != "enable [podNames...]" { | ||
| t.Fatalf("Use = %q, want %q", cmd.Use, "enable [podNames...]") | ||
| } | ||
| } | ||
|
|
||
| func TestNewDisableCmd(t *testing.T) { | ||
| cmd := NewDisableCmd() | ||
| if cmd.Use != "disable [podNames...]" { | ||
| t.Fatalf("Use = %q, want %q", cmd.Use, "disable [podNames...]") | ||
| } | ||
| } | ||
|
|
||
| func TestNewStatusCmd(t *testing.T) { | ||
| cmd := NewStatusCmd() | ||
| if cmd.Use != "status [podNames...]" { | ||
| t.Fatalf("Use = %q, want %q", cmd.Use, "status [podNames...]") | ||
| } | ||
| } |
There was a problem hiding this comment.
While these tests are good for verifying the command structure, they don't test the execution logic within the Run functions of the commands (e.g., for enable, disable, status). This leaves the core functionality of the authz subcommand untested.
Consider refactoring the authz package to separate the command logic from the Kubernetes client and port-forwarding setup. This would allow you to write unit tests for the logic using an httptest server, similar to how it's done in ctl/log/log_test.go. This would make the tests truly comprehensive.
For example, you could add tests that execute the command and verify that the correct HTTP request is made to the mock server.
Eeshu-Yadav
force-pushed
the
test/kmeshctl-ut-e2e-991
branch
from
4142712 to
9f0797c
Compare
There was a problem hiding this comment.
Pull request overview
Adds missing unit test coverage for kmeshctl CLI subcommands, expanding beyond the previously existing version tests by validating cobra command wiring, flags, and a few pure-function/output behaviors without requiring a live Kubernetes cluster.
Changes:
- Added unit tests for root command subcommand registration (
ctl/common). - Added unit tests for multiple subcommands’ cobra structure/flags and helper functions (
dump,log,monitoring,authz,waypoint). - Added output/HTTP behavior tests using
httptestand stdout capture where needed.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ctl/waypoint/waypoint_test.go | Tests waypoint namespace defaulting, subcommand registration, persistent flags, and Args validation. |
| ctl/monitoring/monitoring_test.go | Tests daemon pod arg parsing helper and monitoring command flags. |
| ctl/log/log_test.go | Tests JSON fetch helper and logger get/set behaviors via httptest, plus command flag presence. |
| ctl/dump/dump_test.go | Tests IP conversion helper, table printing output behavior, invalid JSON fallback, and dump command flags. |
| ctl/common/common_test.go | Tests root command config and expected subcommand registration. |
| ctl/authz/authz_test.go | Tests authz command tree structure and Use strings for subcommands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Eeshu-Yadav
force-pushed
the
test/kmeshctl-ut-e2e-991
branch
from
9f0797c to
da0a465
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Eeshu-Yadav
force-pushed
the
test/kmeshctl-ut-e2e-991
branch
from
b1a3131 to
90538d5
Compare
|
Please update your base branch. |
updating it |
Eeshu-Yadav
force-pushed
the
test/kmeshctl-ut-e2e-991
branch
from
90538d5 to
a1cfb44
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Add comprehensive unit tests for kmeshctl covering: - common: root command subcommand registration - dump: uint32ToIPStr, printDualEngineTable, command flags - log: GetJson, GetLoggerNames, GetLoggerLevel, SetLoggerLevel via httptest - monitoring: getKmeshDaemonPod arg parsing, command flags - authz: command tree structure (enable/disable/status) - waypoint: namespaceOrDefault, subcommand registration, args validation Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
Eeshu-Yadav
force-pushed
the
test/kmeshctl-ut-e2e-991
branch
from
a1cfb44 to
6af59d7
Compare
Test kernel-native config dump table formatting using real protobuf ConfigDump types, covering populated data, empty dump, and invalid JSON fallback. Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
Eeshu-Yadav
force-pushed
the
test/kmeshctl-ut-e2e-991
branch
from
6af59d7 to
3f28612
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| want := map[string]bool{ | ||
| "log": false, "dump": false, "waypoint": false, "version": false, | ||
| "monitoring": false, "authz": false, "secret": false, | ||
| } |
There was a problem hiding this comment.
PR title/description says unit tests were added for all kmeshctl subcommands, but this PR still has no tests for the secret subcommand (ctl/secret has no *_test.go). Either add a basic ctl/secret/secret_test.go (e.g., validates command tree/flags like other packages) or adjust the PR title/description to exclude secret from the claim.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds comprehensive unit tests for
kmeshctlCLI tool. Previously onlyversion/version_test.goexisted. This PR covers all remaining subcommands.New test files:
commondumpuint32ToIPStr,printDualEngineTable, command flagslogGetJson,GetLoggerNames,GetLoggerLevel,SetLoggerLevelvia httptestmonitoringgetKmeshDaemonPodarg parsing, command flagsauthzwaypointnamespaceOrDefault, subcommand registration, args validationAll tests run without a Kubernetes cluster using pure function tests,
httptestservers, and cobra command structure validation.Which issue(s) this PR fixes:
Closes #991