[BUG] make PicoD execute tests portable#308
[BUG] make PicoD execute tests portable#308krrishrastogi05 wants to merge 1 commit intovolcano-sh: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 |
![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 refactors the test suite to use a self-invoking helper process instead of relying on external shell commands, which improves test portability. The review feedback correctly identifies that the current implementation using -test.run will pollute stdout with test runner logs, potentially breaking assertions. It is recommended to refactor the helper logic into an init() function and use os.Executable() to ensure a reliable path to the test binary and clean output.
| func testCommand(args ...string) []string { | ||
| command := []string{os.Args[0], "-test.run=TestPicodHelperProcess", "--"} | ||
| return append(command, args...) | ||
| } |
There was a problem hiding this comment.
Using os.Args[0] and -test.run for the helper process has two significant drawbacks:
- Portability/Reliability:
os.Args[0]can be unreliable if the working directory is changed during tests (as seen in several test cases) and it contains a relative path. Usingos.Executable()provides a more robust absolute path to the test binary. - Test Runner Noise: Invoking the helper via
-test.runcauses the Go test runner to emit=== RUN TestPicodHelperProcesstostdout. This extra output will be captured inresp.Stdout, causing assertions that expect exact matches (e.g.,assert.Equal(t, "hello\n", resp.Stdout)) to fail.
It is recommended to move the helper logic trigger to an init() function and use os.Executable() for the command path. This bypasses the test runner's output entirely.
| func testCommand(args ...string) []string { | |
| command := []string{os.Args[0], "-test.run=TestPicodHelperProcess", "--"} | |
| return append(command, args...) | |
| } | |
| func init() { | |
| if os.Getenv(testHelperProcessEnv) == "1" { | |
| picodHelperProcessMain() | |
| os.Exit(0) | |
| } | |
| } | |
| func testCommand(args ...string) []string { | |
| exe, err := os.Executable() | |
| if err != nil { | |
| exe = os.Args[0] | |
| } | |
| return append([]string{exe, "--"}, args...) | |
| } |
There was a problem hiding this comment.
Pull request overview
This PR makes PicoD’s test suite more portable across OSes (notably Windows) by removing dependencies on Unix shell utilities and by handling Windows symlink privilege limitations in tests, without changing PicoD production execution behavior.
Changes:
- Introduces a Go “helper process” test binary entrypoint and helpers to replace host-specific commands like
sh,sleep,pwd,true, andfalse. - Updates PicoD end-to-end and handler tests to execute commands via the helper process and to consistently inject the helper env var.
- Adds a symlink helper that skips tests on Windows when symlink creation is denied.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/picod/test_helpers_test.go | Adds helper-process command runner helpers and a Windows-aware symlink helper for portable tests. |
| pkg/picod/picod_test.go | Switches E2E execute tests to use the helper-process commands and uses the symlink helper. |
| pkg/picod/execute_test.go | Replaces OS-specific commands in ExecuteHandler tests with helper-process commands and adjusts env usage. |
| func TestExecuteHandler_EmptyEnvVars(t *testing.T) { | ||
| server, tmpDir := setupExecuteTestServer(t) | ||
| defer os.RemoveAll(tmpDir) | ||
| defer os.Unsetenv(PublicKeyEnvVar) | ||
|
|
||
| req := ExecuteRequest{ | ||
| Command: []string{"echo", "test"}, | ||
| Env: map[string]string{}, | ||
| Command: testCommand("echo", "test"), | ||
| Env: testCommandEnv(nil), | ||
| } |
| func requireSymlink(t *testing.T, oldname, newname string) { | ||
| t.Helper() | ||
| if err := os.Symlink(oldname, newname); err != nil { | ||
| if runtime.GOOS == "windows" { | ||
| t.Skipf("creating symlinks requires privileges on Windows: %v", err) | ||
| } | ||
| t.Fatalf("create symlink: %v", err) | ||
| } |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #308 +/- ##
==========================================
+ Coverage 47.57% 47.75% +0.18%
==========================================
Files 30 30
Lines 2819 2854 +35
==========================================
+ Hits 1341 1363 +22
- Misses 1338 1343 +5
- Partials 140 148 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Krrish <krrishrastogi00@gmail.com>
krrishrastogi05
force-pushed
the
krrish/picod-portable-tests
branch
from
f27efad to
9b516eb
Compare
/kind cleanup
What this PR does / why we need it:
While testing a small PicoD change locally on Windows, I noticed that
go test ./pkg/picodfailed for reasons unrelated to the code under test. Some tests assumed Unix tools likesh,sleep,pwd,true, andfalsewere available, and symlink tests failed when Windows denied symlink creation privileges.This PR makes those tests more portable without changing PicoD runtime behavior. It should make the project a bit more welcoming for contributors who are developing on Windows, since they can run the PicoD test suite locally without needing a Unix-like shell setup or elevated symlink permissions.
The main changes are:
Special notes for your reviewer:
This is a test-only cleanup. The production PicoD command execution path is unchanged.
Testing done:
gofmtgit diff --checkgo test ./pkg/picodDoes this PR introduce a user-facing change?: