test: add unit and e2e coverage for log package#1761
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 automated test coverage for the Kmesh log package, addressing issue #1235. By implementing both unit tests for CLI logic and E2E tests for daemon interaction, the changes ensure robust validation of logger discovery and configuration updates without introducing production code modifications or test-only hooks. Highlights
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 the 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 counterproductive. 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. Logs are checked with care and pride, / In unit tests where truths reside. / With E2E the flow is clear, / No bugs shall haunt the Kmesh gear. Footnotes
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds automated coverage for kmeshctl log behavior via a new integration/e2e test and new unit tests around the underlying HTTP-based log helpers.
Changes:
- Added an e2e integration test validating
kmeshctl loglist/get/set flows against a real Kmesh daemon pod. - Added unit tests for logger name listing, logger level retrieval, and logger level updates using
httptest.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/e2e/log_test.go | New integration test exercising kmeshctl log against a live pod and verifying outputs/state. |
| ctl/log/log_test.go | New unit tests for log helper functions using an HTTP test server and captured stdout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| output := string(out) | ||
| // Note: actual output depends on daemon response, typically empty or success message. | ||
| // We check if it ran without errors. | ||
| return nil |
| cmd := exec.Command("kmeshctl", "log", podName) | ||
| out, err := cmd.CombinedOutput() |
| func captureStdout(f func()) string { | ||
| oldStdout := os.Stdout | ||
| r, w, _ := os.Pipe() | ||
| os.Stdout = w | ||
|
|
||
| f() | ||
|
|
||
| w.Close() | ||
| os.Stdout = oldStdout | ||
|
|
||
| var buf bytes.Buffer | ||
| io.Copy(&buf, r) | ||
| return buf.String() | ||
| } |
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(tt.mockStatus) | ||
| w.Write([]byte(tt.mockBody)) | ||
| })) |
| if tt.expectedStdout != "" && !strings.Contains(output, tt.expectedStdout) { | ||
| t.Errorf("expected stdout to contain %q, got %q", tt.expectedStdout, output) | ||
| } |
![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 introduces unit and end-to-end integration tests for the kmeshctl log command, covering retrieving logger names, getting logger levels, and setting logger levels. The review feedback highlights two main improvements: first, ensuring that os.Stdout is safely restored and resources are cleaned up in captureStdout using defer to handle potential panics; second, adding a guard check in the end-to-end test cleanup to prevent executing an invalid command if retrieving the original logger level fails.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func captureStdout(f func()) string { | ||
| oldStdout := os.Stdout | ||
| r, w, _ := os.Pipe() | ||
| os.Stdout = w | ||
|
|
||
| f() | ||
|
|
||
| w.Close() | ||
| os.Stdout = oldStdout | ||
|
|
||
| var buf bytes.Buffer | ||
| io.Copy(&buf, r) | ||
| return buf.String() | ||
| } |
There was a problem hiding this comment.
If the function f panics during execution, os.Stdout will not be restored to its original value, and the write end of the pipe w will not be closed. This can hijack os.Stdout for the rest of the test suite run, causing subsequent tests to fail or lose their output, and leak file descriptors.
Using defer to restore os.Stdout and close the pipe files ensures that resources are cleaned up and global state is restored even if a panic occurs.
func captureStdout(f func()) string {
oldStdout := os.Stdout
r, w, err := os.Pipe()
if err != nil {
panic(err)
}
os.Stdout = w
defer func() {
os.Stdout = oldStdout
_ = r.Close()
}()
func() {
defer w.Close()
f()
}()
var buf bytes.Buffer
_, _ = io.Copy(&buf, r)
return buf.String()
}| defer func() { | ||
| if err := exec.Command( | ||
| "kmeshctl", | ||
| "log", | ||
| podName, | ||
| "--set", | ||
| "default:"+originalLevel, | ||
| ).Run(); err != nil { | ||
| t.Logf("cleanup failed to restore original logger level (%s): %v", originalLevel, err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
If the setup step fails or times out, originalLevel will remain empty (""). Running the cleanup command with an empty level (default:) will result in an invalid command execution, which can cause confusing test logs or unexpected daemon behavior. Adding a guard check to ensure originalLevel is not empty before executing the restore command prevents this.
defer func() {
if originalLevel == "" {
return
}
if err := exec.Command(
"kmeshctl",
"log",
podName,
"--set",
"default:"+originalLevel,
).Run(); err != nil {
t.Logf("cleanup failed to restore original logger level (%s): %v", originalLevel, err)
}
}()Signed-off-by: Ravindra Kumar Bundela <ravindrarkb1205@gmail.com>
ravindra-RKB
force-pushed
the
test/log-e2e-test
branch
from
3af6c26 to
6c5eb11
Compare
|
Addressed the review feedback: Removed the compile-blocking unused variable in The branch has been force-pushed with the fixes. Please take another look when you have a chance. Thanks! |
What type of PR is this?
kind enhancement
What this PR does / why we need it
This PR adds automated test coverage for the Kmesh log package and resolves Issue #1235.
The implementation is split into two layers:
ctl/log/log_test.go)**Validate
GetLoggerNames,GetLoggerLevel, andSetLoggerLevel.Use
httptest.NewServerto simulate daemon responses.Verify JSON parsing, HTTP error handling, and CLI output formatting.
Avoid modifications to production code and do not introduce test-only hooks.
test/e2e/log_test.go)**Validate the complete
kmeshctl logworkflow against a live Kmesh daemon.Verify logger discovery.
Verify logger level retrieval.
Verify runtime log level updates.
Restore the original logger level after test execution to avoid side effects.
Which issue(s) this PR fixes
Fixes #1235
Special notes for your reviewer
No production code was modified.
The implementation avoids the mock-injection approach used in previous attempts.
E2E tests use the existing Kmesh/Istio test framework and retry utilities.
Cleanup logic restores the original logger level instead of hardcoding a default value.
Unit tests intentionally avoid
t.Parallel()because stdout is captured during execution.Does this PR introduce a user-facing change?