test(e2e/up): organize tests #355
Conversation
Signed-off-by: Samuel K <skevetter@pm.me>
📝 WalkthroughWalkthroughReplaces and reorganizes the "up" end-to-end tests: removes legacy Docker/Podman suites and a test helper, adds provider- and feature-specific test files and helpers, and renames/adjusts CI job labels and resource flags in Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as DevPod CLI
participant Harness as Test Harness
participant Provider as Provider (Docker/Podman/K8s)
participant Runtime as Container/Pod Runtime
participant SSH as SSH into workspace
CLI->>Harness: invoke DevPodUp (test)
Harness->>Provider: request workspace up / provision
Provider->>Runtime: pull/build image, create container/pod (mounts, env)
Runtime-->>Provider: pod/container running
Provider-->>Harness: workspace endpoint & metadata
Harness->>SSH: run verification commands
SSH-->>Harness: outputs / assertions
Harness->>Provider: stop/delete workspace
Provider->>Runtime: remove pod/container
Runtime-->>Provider: removed
Provider-->>Harness: confirm cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 15
🤖 Fix all issues with AI agents
In `@e2e/tests/up/docker_wsl.go`:
- Around line 226-230: The test's skip condition is inverted: inside the
Windows-only suite the code currently skips when runtime.GOOS == "windows". In
the ginkgo.It block for "should start a new workspace with dotfiles - no install
script, branch" change the condition to skip when runtime.GOOS != "windows" (or
remove the skip entirely if the suite shouldn't gate by OS) and update the skip
message accordingly so it reads something like ginkgo.Skip("skipping on
non-windows") when not running on Windows.
- Around line 35-36: The framework.ExpectNoError(err) call is dead here because
dockerHelper = &docker.DockerHelper{...} does not produce an error and err still
refers to the earlier os.Getwd() result already checked; remove the redundant
framework.ExpectNoError(err) line (the reference to framework.ExpectNoError(err)
should be deleted) so only the relevant error check for os.Getwd() remains and
dockerHelper is assigned directly using docker.DockerHelper.
- Around line 203-207: The inner skip inside the ginkgo.It ("should start a new
workspace with dotfiles - no install script, commit") contradicts the
suite-level BeforeEach that already restricts the suite to Windows (BeforeEach
with runtime.GOOS != "windows"), so remove the inner runtime.GOOS == "windows"
Skip (or invert it if the intent is to skip Windows) so the test can actually
run on the platform intended; locate the test by the ginkgo.It description
string and the surrounding BeforeEach and delete or adjust the runtime.GOOS
check accordingly (alternatively, use ginkgo.PIt to mark it pending if it truly
shouldn't run on Windows).
In `@e2e/tests/up/dockerfile_build.go`:
- Around line 111-122: The script file remains open when calling f.DevPodUp
which can cause rebuild failures on Windows; close scriptFile before invoking
DevPodUp. After writing to scriptFile (the scriptFile variable returned from
os.OpenFile and used with scriptFile.Write), call scriptFile.Close() (or defer
was earlier but ensure explicit Close() after write) and handle the error, then
proceed to call f.DevPodUp(ctx, tempDir, "--debug", "--recreate"); ensure any
defer close is not the only close preventing the file from being flushed/closed
prior to the rebuild.
- Around line 70-81: The script file is left open via deferred close which can
block Docker on Windows; explicitly close scriptFile (the os.File written to at
scriptPath) after writing and checking the write error, before calling
f.DevPodUp(ctx, tempDir, "--debug", "--recreate"); remove or convert the defer
that closes scriptFile so you don’t close twice and ensure
framework.ExpectNoError(err) is used after the explicit Close() to catch any
Close() errors.
- Around line 27-28: Remove the misleading error assertion by deleting the
redundant framework.ExpectNoError(err) after initializing dockerHelper; the err
being checked is from os.Getwd() (used earlier) and not related to
docker.DockerHelper initialization, so either move the ExpectNoError(err) to
immediately follow the os.Getwd() call or remove the later call that follows
dockerHelper = &docker.DockerHelper{...}, ensuring only the correct error
variable is asserted and no dead check remains (look for symbols: os.Getwd, err,
dockerHelper, DockerHelper, framework.ExpectNoError).
In `@e2e/tests/up/git_repositories.go`:
- Around line 34-45: The test case in the ginkgo.It block uses a fragile fork PR
ref ("github.com/skevetter/devpod@pull/100/head") when calling f.DevPodUp;
update the f.DevPodUp invocation in that test (the ginkgo.It anonymous function
using ctx, f from setupDockerProvider and name "pr100") to point to a stable
source such as the main repository branch or a dedicated test repo/commit (e.g.,
main or a pinned commit) so the test doesn't break when forked PR refs change or
are deleted.
In `@e2e/tests/up/handles_errors.go`:
- Around line 22-46: Tests after the first one create a docker provider via
setupDockerProvider but never clean it up, causing dangling providers; update
the tests to call ginkgo.DeferCleanup to delete the provider (using
f.DevPodProviderDelete or the existing DevPodProviderDelete wrapper) after
setup, or modify setupDockerProvider to register a cleanup function itself (call
f.DevPodProviderAdd inside setupDockerProvider and call
ginkgo.DeferCleanup(func(){ f.DevPodProviderDelete(context.Background(),
"<provider-name>") })) so every test that uses setupDockerProvider automatically
removes the provider; reference the provider operations DevPodProviderAdd and
DevPodProviderDelete and the test helper setupDockerProvider when making the
change.
In `@e2e/tests/up/provider_docker.go`:
- Around line 102-104: The assertion builds the expected path incorrectly
(missing leading slash) for containerWorkspaceFolder; update the expectation to
include the leading slash by constructing the path with filepath.Join("/",
"workspaces", filepath.Base(tempDir)) (or otherwise prefixing "/" to the joined
path) and keep using framework.CleanString(containerWorkspaceFolder) and
framework.CleanString(...) for comparison so the test compares
"/workspaces/<basename>" to the captured value from execSSHCapture.
In `@e2e/tests/up/provider_kubernetes.go`:
- Around line 22-95: The test "runs devpod in Kubernetes" (ginkgo.It block) is
missing a timeout decorator and can hang CI; add
ginkgo.SpecTimeout(framework.GetTimeout()) to the test declaration so the It
block has a timeout (i.e., change the ginkgo.It invocation that wraps the
function to include ginkgo.SpecTimeout(framework.GetTimeout()) as a
modifier/decorator before the test function) to match other tests and ensure the
test will fail fast on hangs.
In `@e2e/tests/up/up_private_token.go`:
- Around line 35-39: The .git-credentials file is created with overly-permissive
permissions; update the os.WriteFile call that writes gitCredentialString to the
path produced by filepath.Join(os.Getenv("HOME"), ".git-credentials") to use
mode 0o600 instead of 0o644 so the file is owner-readable/writable only (locate
the os.WriteFile invocation and change the permission argument).
In `@e2e/tests/up/up.go`:
- Line 288: Update the inline comment that misspells the CLI flag: find the
comment containing "--recreare" and change it to "--recreate" so the comment
correctly reads that the command should fail because "--recreate" triggers a new
build and rootfs; this is the only change needed in the comment in
e2e/tests/up/up.go.
- Around line 282-283: The test call to f.DevPodUpReset uses an inconsistent URL
with an extra slash before `@subpath`; update the argument passed to
f.DevPodUpReset in up.go to use the same format as the other calls (e.g.,
"https://github.com/loft-sh/examples@subpath:/devpod/jupyter-notebook-hello-world"
instead of ".../examples/@subpath:...") so the resource targeted matches the
other lines (see other usages around DevPodUpReset/DevPodUp calls).
- Around line 27-28: The ExpectNoError(err) after initializing dockerHelper is
redundant because err still refers to the earlier os.Getwd() call and not the
DockerHelper assignment; remove the framework.ExpectNoError(err) following
dockerHelper = &docker.DockerHelper{DockerCommand: "docker", Log: log.Default}
(or if you intended to check an error from DockerHelper construction, change the
code to capture and check that error from the actual constructor), ensuring only
the correct error variable is checked.
- Around line 264-269: DeferCleanup currently calls f.DevPodWorkspaceDelete with
the hard-coded "jupyter-notebook-hello-world" which doesn't match the actual
workspace id "subpath--devpod-jupyter-notebook-hello-world" and is unnecessary
because the workspace is already deleted later; update the ginkgo.DeferCleanup
to remove the workspace deletion call and only call f.DevPodProviderDelete(ctx,
providerName) (leaving framework.ExpectNoError(err) for that call) so cleanup
only handles provider removal and avoids a mismatched/or duplicate workspace
delete.
🧹 Nitpick comments (13)
e2e/tests/up/provider_podman.go (2)
49-59: Wrapper script has embedded indentation and uses unnecessary sudo for chmod.The shell script content includes leading tabs/spaces that will be part of the script. Also, since the file was just created by the current process,
os.Chmodshould work without sudo. Usingsudo chmodadds an unnecessary dependency on passwordless sudo.Proposed fix
- _, err = wrapper.WriteString(`#!/bin/sh - sudo podman "$@" - `) + _, err = wrapper.WriteString("#!/bin/sh\nsudo podman \"$@\"\n") framework.ExpectNoError(err) err = wrapper.Close() framework.ExpectNoError(err) - cmd := exec.Command("sudo", "chmod", "+x", initialDir+"/bin/podman-rootful") - err = cmd.Run() + err = os.Chmod(initialDir+"/bin/podman-rootful", 0755) framework.ExpectNoError(err)
43-63: Missing cleanup for the wrapper script file.The
podman-rootfulwrapper script is created inBeforeAllbut never cleaned up. Consider adding aDeferCleanupto remove it after the test context completes.Proposed fix
err = exec.Command(initialDir+"/bin/podman-rootful", "ps").Run() framework.ExpectNoError(err) + + ginkgo.DeferCleanup(func() { + _ = os.Remove(initialDir + "/bin/podman-rootful") + }) })e2e/tests/up/provider_kubernetes.go (2)
42-44: Useexec.CommandContextfor cancellation support.The kubectl commands use
exec.Commandwhich won't be cancelled if the context times out. Usingexec.CommandContext(ctx, ...)ensures the command is terminated when the test context is cancelled.Proposed fix (apply to all three kubectl calls)
- cmd := exec.Command("kubectl", "get", "pods", "-l", "devpod.sh/created=true", "-o", "json", "-n", "devpod") + cmd := exec.CommandContext(ctx, "kubectl", "get", "pods", "-l", "devpod.sh/created=true", "-o", "json", "-n", "devpod")
37-39: Workspace cleanup should use DeferCleanup.The workspace deletion at line 93 won't execute if any assertion fails earlier in the test. Register workspace cleanup via
DeferCleanupafterDevPodUpsucceeds to ensure cleanup occurs even on test failure.Proposed fix
// run up err = f.DevPodUp(ctx, tempDir) framework.ExpectNoError(err) + ginkgo.DeferCleanup(f.DevPodWorkspaceDelete, tempDir) // check pod is thereAnd remove the explicit delete at the end:
- // delete workspace - err = f.DevPodWorkspaceDelete(ctx, tempDir) - framework.ExpectNoError(err) })e2e/tests/up/handles_errors.go (2)
48-61: Missing SpecTimeout on test case.Unlike the first test case (line 46) and the third test case (line 76), this test case does not have a
ginkgo.SpecTimeout. Consider adding it for consistency and to prevent tests from hanging indefinitely.Suggested fix
ginkgo.It("ensure workspace cleanup when failing to create a workspace", func(ctx context.Context) { f, err := setupDockerProvider(initialDir+"/bin", "docker") framework.ExpectNoError(err) initialList, err := f.DevPodList(ctx) framework.ExpectNoError(err) // Wait for devpod workspace to come online (deadline: 30s) err = f.DevPodUp(ctx, "github.com/i/do-not-exist.git") framework.ExpectError(err) out, err := f.DevPodList(ctx) framework.ExpectNoError(err) framework.ExpectEqual(out, initialList) - }) + }, ginkgo.SpecTimeout(framework.GetTimeout()))
78-91: Missing SpecTimeout on test case.This test case is also missing
ginkgo.SpecTimeoutfor consistency with other tests in this file.Suggested fix
ginkgo.It("ensure workspace cleanup when not a git or folder", func(ctx context.Context) { f, err := setupDockerProvider(initialDir+"/bin", "docker") framework.ExpectNoError(err) initialList, err := f.DevPodList(ctx) framework.ExpectNoError(err) // Wait for devpod workspace to come online (deadline: 30s) err = f.DevPodUp(ctx, "notfound.loft.sh") framework.ExpectError(err) out, err := f.DevPodList(ctx) framework.ExpectNoError(err) framework.ExpectEqual(out, initialList) - }) + }, ginkgo.SpecTimeout(framework.GetTimeout()))e2e/tests/up/up_private_token.go (2)
49-51: Use GinkgoWriter instead of fmt.Println for test output.Using
fmt.Printlnbypasses Ginkgo's output handling. Useginkgo.GinkgoWriterorfmt.Fprintln(ginkgo.GinkgoWriter, out)to ensure output is properly captured and displayed according to Ginkgo's verbosity settings.
23-52: Missing SpecTimeout.This test performs network operations (git clone from private repo) but lacks a
SpecTimeout. Consider adding one for consistency with other tests.e2e/tests/up/dockerfile_build.go (1)
17-32: ginkgo.Ordered with BeforeEach may cause test isolation issues.The test suite is marked as
ginkgo.Ordered, butBeforeEachreinitializesfanddockerHelperbefore each test. WithOrdered, consider whetherBeforeAllwould be more appropriate if the provider setup is intended to be shared, or verify that the current setup correctly isolates tests while maintaining order dependency.e2e/tests/up/provider_docker.go (2)
131-143: Redundant assertion - ExpectNotEqual after ExpectEqual.Line 142 asserts that
outis not equal to"ID=debian\n", but this is redundant given line 141 already asserts it equals"ID=alpine\n". If both assertions were to pass, they would be mutually exclusive. Consider removing the redundant check.Suggested fix
out, err := dtc.execSSH(ctx, tempDir, "grep ^ID= /etc/os-release") framework.ExpectNoError(err) framework.ExpectEqual(out, "ID=alpine\n") - framework.ExpectNotEqual(out, "ID=debian\n")
145-153: Same redundant assertion pattern.Lines 151-152 have the same redundant
ExpectNotEqualafterExpectEqual.e2e/tests/up/up.go (1)
18-29: Missing SpecTimeout on all test cases.None of the test cases in this file have
ginkgo.SpecTimeout. Given that these tests involve network operations, container creation, and SSH connections, adding timeouts would prevent tests from hanging indefinitely.e2e/tests/up/git_repositories.go (1)
21-45: Inconsistent provider setup and missing SpecTimeout.The first two tests use
setupDockerProviderwhile the third test manually sets up the provider. For consistency, consider using the same approach across all tests. Additionally, all three tests are missingginkgo.SpecTimeout.
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@e2e/tests/up/docker_wsl.go`:
- Around line 19-34: This test suite runs Windows/WSL-specific logic but lacks a
platform guard; update the ginkgo.BeforeEach in the Describe block (the
BeforeEach anonymous func) to skip when runtime.GOOS != "windows" (use runtime
package and call ginkgo.Skip) or add a file-level build constraint //go:build
windows (matching up-features/wsl.go) so the
Describe/ginkgo.Label("up-docker-wsl") suite only runs on Windows; ensure you
reference and preserve dockerHelper, originalDockerHost and DOCKER_HOST setup
when applying the guard.
In `@e2e/tests/up/git_repositories.go`:
- Around line 34-45: The test case description and behavior diverge: the
ginkgo.It titled "should allow checkout of a GitRepo from a pull request
reference" creates name := "pr100" but calls f.DevPodUp(ctx,
"github.com/loft-sh/devpod") with no PR ref; update the test to make intent
explicit by either (A) changing the DevPodUp call to include a PR ref (e.g., use
the repository URL with a pull ref like "@pull/100/head" or whichever PR ref
format DevPodUp expects) and keep name := "pr100", or (B) rename the ginkgo.It
title and the name variable to reflect a plain repo checkout (e.g., "should
allow checkout of a GitRepo from main branch" and name := "devpod-main"); adjust
any cleanup calls (ginkgo.DeferCleanup f.DevPodWorkspaceDelete) as needed.
Ensure you modify the single test invocation that uses f.DevPodUp to match the
chosen option so the test name, name variable, and f.DevPodUp call are
consistent.
In `@e2e/tests/up/provider_kubernetes.go`:
- Around line 62-71: The direct kubectl get + immediate framework.ExpectEqual
pod-count checks race with async pod creation/deletion; replace each immediate
assertion (the blocks that run exec.CommandContext("kubectl", "get", "pods",
...), unmarshal into list, then framework.ExpectEqual(len(list.Items), ...))
with a gomega.Eventually polling closure that runs the kubectl command,
unmarshals into the same list variable, and returns len(list.Items), then use
gomega.Equal(expected) with a sensible timeout and poll interval (e.g.,
30s/500ms) — apply this pattern for the three locations: after the first
DevPodUp, after DevPodWorkspaceStop, and after the second DevPodUp, keeping the
existing cmd/stdout/list variables and error checks inside the closure so the
tests wait for pod state to stabilize before asserting.
♻️ Duplicate comments (1)
e2e/tests/up/provider_kubernetes.go (1)
22-24: Add a spec timeout and use Ginkgo’s context.This
Itstill lacksginkgo.SpecTimeout(...)and usescontext.Background(), so long-runningkubectl/devpodcalls won’t be bounded or cancelled if the spec times out. (Same concern as earlier review.)Ginkgo v2 SpecTimeout: how to pass context to It and use it for exec.CommandContext cancellation?
🧹 Nitpick comments (4)
e2e/tests/up/dockerfile_build.go (1)
44-48: Misleading comment about deadline.The comment "deadline: 30s" is inconsistent with the actual
SpecTimeout(framework.GetTimeout()*3)which is 360s on non-Windows (or 1800s on Windows). Consider removing or correcting this comment.Suggested fix
- // Wait for devpod workspace to come online (deadline: 30s) + // Wait for devpod workspace to come online err = f.DevPodUp(ctx, tempDir, "--debug")e2e/tests/up/up.go (2)
30-141: Comprehensive env var test with potential timeout concern.This test performs numerous DevPodUp, DevPodSSH operations, file I/O, and error case validation. The single
GetTimeout()multiplier may be tight for this extensive workflow.Consider using a higher timeout multiplier (e.g.,
GetTimeout()*2) given the test's complexity.
157-161: Redundant workspace deletion.The workspace is deleted twice: once in DeferCleanup (line 158) and again explicitly at the end of the test (line 184). While not harmful (the second deletion will likely be a no-op or fail silently), it's unnecessary duplication.
Suggested fix - remove from DeferCleanup
ginkgo.DeferCleanup(func() { - _ = f.DevPodWorkspaceDelete(ctx, tempDir) err = f.DevPodProviderDelete(ctx, providerName) framework.ExpectNoError(err) })Also applies to: 184-185
e2e/tests/up/docker_wsl.go (1)
36-37: Use filepath.Join for provider bin path.
String concatenation for filesystem paths is non‑idiomatic and can be brittle across platforms. Preferfilepath.Join.♻️ Proposed change
- f, err = setupDockerProvider(initialDir+"/bin", "docker") + f, err = setupDockerProvider(filepath.Join(initialDir, "bin"), "docker")
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@e2e/tests/up/up.go`:
- Around line 276-281: The comment on the failing ExpectError for the ROOTFS
check incorrectly mentions "--recreate"; update the comment to reference the
actual flag used by this test (DevPodUpReset) i.e. "--reset" behavior so it
matches the call to DevPodUpReset and the subsequent DevPodSSH/assertion; ensure
both comments above the two ExpectError assertions consistently state that
"--reset" triggers a fresh git clone and new rootfs respectively.
- Around line 173-179: The code builds a container path using filepath.Join
which uses OS-specific separators; replace filepath.Join with path.Join when
constructing devcontainerPath (the variable created from
filepath.Join("/workspaces", projectName, ".devcontainer.json")) to ensure
forward-slash Linux-style paths in the container environment; update the call to
use path.Join and import the "path" package if not already imported.
♻️ Duplicate comments (1)
e2e/tests/up/docker_wsl.go (1)
19-46: Reintroduce a Windows-only guard for this WSL suite.Line 26 sets a WSL-specific
DOCKER_HOST, but there’s no runtime gate; if this suite runs on non‑Windows runners it will fail. Please restore the OS guard (or add a build tag) unless CI now guarantees Windows-only execution.🛠️ Suggested guard (runtime skip)
import ( "context" "fmt" "os" "path/filepath" + "runtime" "github.com/docker/docker/api/types/container" @@ ginkgo.BeforeEach(func(ctx context.Context) { + if runtime.GOOS != "windows" { + ginkgo.Skip("skipping on non-windows") + } + initialDir, err = os.Getwd() framework.ExpectNoError(err)
🧹 Nitpick comments (3)
e2e/tests/up/provider_docker.go (2)
20-32: Usefilepath.Joinfor path construction.Line 30 uses string concatenation (
dtc.initialDir+"/bin") to build the path, which is inconsistent with best practices for cross-platform compatibility.Suggested fix
- dtc.f, err = setupDockerProvider(dtc.initialDir+"/bin", "docker") + dtc.f, err = setupDockerProvider(filepath.Join(dtc.initialDir, "bin"), "docker")
102-104: Usepath.Joinfor container paths instead offilepath.Join.Container paths are always Linux-style (forward slashes), regardless of the host OS. Using
filepath.Joinwill produce backslashes on Windows, which won't match the container's Linux paths.Suggested fix
containerWorkspaceFolder, err := dtc.execSSHCapture(ctx, workspace.ID, "cat $HOME/container-workspace-folder.out") framework.ExpectNoError(err) - gomega.Expect(framework.CleanString(containerWorkspaceFolder)).To(gomega.Equal(framework.CleanString(filepath.Join("/", "workspaces", filepath.Base(tempDir))))) + gomega.Expect(framework.CleanString(containerWorkspaceFolder)).To(gomega.Equal(framework.CleanString(path.Join("/workspaces", filepath.Base(tempDir)))))e2e/tests/up/up.go (1)
30-32: Usefilepath.Joinfor path construction.Same issue as in
provider_docker.go- string concatenation for paths is not cross-platform safe.Suggested fix
- f, err := setupDockerProvider(initialDir+"/bin", "docker") + f, err := setupDockerProvider(filepath.Join(initialDir, "bin"), "docker")
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@e2e/tests/up/provider_podman.go`:
- Around line 42-65: The BeforeEach setup creates a fixed wrapper at
initialDir+"/bin/podman-rootful", calls sudo without -n and uses exec.Command,
causing parallel-test conflicts and potential blocking; change it to create a
per-test temp bin directory (use os.MkdirTemp or similar) and write the wrapper
there (unique name per test), update the wrapper content to use "sudo -n podman
\"$@\"" to avoid password prompts, replace exec.Command(...) with
exec.CommandContext(ctx, ...) when invoking the wrapper (ps call) and when
passing that temp bin path into setupDockerProvider(initialBinDir, wrapperPath),
and ensure cleanup removes the temp bin directory in ginkgo.DeferCleanup.
In `@e2e/tests/up/up.go`:
- Around line 75-87: CreateTempDir() returns a temporary directory that is not
being removed; after calling framework.CreateTempDir() and checking err, add a
cleanup to remove the whole tmpDir (e.g., defer os.RemoveAll(tmpDir)) so the
directory itself does not leak across test runs; place the defer immediately
after framework.ExpectNoError(err) near the existing tmpDir usage (the variable
tmpDir and the invalid env file workspaceEnvFileInvalid in this block) to ensure
the directory and its contents are removed on test exit.
🧹 Nitpick comments (1)
e2e/tests/up/up.go (1)
161-163: Useginkgo.DeferCleanupfor workspace deletion to prevent resource leaks on test failure.Tests at lines 182–183, 211–212, 244–245, and 284–285 delete workspaces only at the end of their test blocks. If an assertion fails earlier, the workspace can leak. The first test in the file correctly uses
ginkgo.DeferCleanupat line 36; apply the same pattern consistently across the other tests.Example pattern (apply similarly in other tests)
err = f.DevPodUp(ctx, tempDir) framework.ExpectNoError(err) + defer func() { + err := f.DevPodWorkspaceDelete(context.Background(), tempDir) + framework.ExpectNoError(err) + }()Also applies to: 211–212, 244–245, 284–285
| ginkgo.BeforeEach(func(ctx context.Context) { | ||
| wrapper, err := os.Create(initialDir + "/bin/podman-rootful") | ||
| framework.ExpectNoError(err) | ||
|
|
||
| defer func() { _ = wrapper.Close() }() | ||
|
|
||
| _, err = wrapper.WriteString("#!/bin/sh\nsudo podman \"$@\"\n") | ||
| framework.ExpectNoError(err) | ||
|
|
||
| err = wrapper.Close() | ||
| framework.ExpectNoError(err) | ||
|
|
||
| err = os.Chmod(initialDir+"/bin/podman-rootful", 0755) | ||
| framework.ExpectNoError(err) | ||
|
|
||
| err = exec.Command(initialDir+"/bin/podman-rootful", "ps").Run() | ||
| framework.ExpectNoError(err) | ||
|
|
||
| ginkgo.DeferCleanup(func() { | ||
| _ = os.Remove(initialDir + "/bin/podman-rootful") | ||
| }) | ||
|
|
||
| f, err = setupDockerProvider(initialDir+"/bin", initialDir+"/bin/podman-rootful") | ||
| framework.ExpectNoError(err) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Go exec.CommandContext context cancellation vs exec.Command
💡 Result:
Short answer
- exec.CommandContext creates a *Cmd whose Cancel is wired to the provided context: when the context is Done() the command is interrupted (by invoking cmd.Cancel, which by default calls Process.Kill()). exec.Command does not do that — you must cancel/kill the process yourself. [1][2]
Important behavioral details
- Default cancellation action: CommandContext sets Cmd.Cancel to call Process.Kill(), so cancellation sends a hard kill (SIGKILL on Unix / Kill on Windows) unless you replace Cmd.Cancel before Start. [1][2]
- Error returned: when the context cancels the process you will typically see the process-exit error (e.g. "signal: killed") rather than context.Canceled. This has been discussed in the Go issue tracker. [11]
- Custom cancellation: if you need graceful shutdown (SIGINT/SIGTERM then timeout → SIGKILL), modify Cmd.Cancel or implement your own watcher instead of using CommandContext directly (several helper libs exist). [9]
- Process groups / children: CommandContext/Process.Kill only kills that Process; spawned child processes may survive. To kill a whole process group on Unix, set SysProcAttr (Setpgid) and send signals to the group. On Windows you need job objects or other APIs. [7][9]
When to use which
- Use exec.CommandContext when you want an easy timeout/cancel that forcefully stops the command on context Done().
- Use exec.Command when you need custom shutdown semantics (graceful signals, wait delays, group-kill behavior) and will implement cancellation yourself.
References
- os/exec CommandContext docs. [1]
- pkg.go.dev os/exec (CommandContext description). [2]
- execctx package (example of customizing cancel behavior). [9]
- Go issue discussing returned error vs. context cancellation. [11]
🌐 Web query:
Ginkgo v2 BeforeEach context parameter cancellation
💡 Result:
Short answer: The context Ginkgo passes into a BeforeEach (a SpecContext / context.Context) is cancelled when that BeforeEach node completes. That means any goroutines you start in BeforeEach must not rely on the node’s ctx staying live after the BeforeEach returns. Use a separate context you control (e.g. context.WithCancel(context.Background())) and register its cancel via DeferCleanup, or make your DeferCleanup node interruptible with its own NodeTimeout/SpecContext. [1][2]
Recommended patterns:
- For background goroutines started in BeforeEach:
- backgroundCtx, cancel := context.WithCancel(context.Background())
- DeferCleanup(cancel)
- go run background goroutine with backgroundCtx.
- If you need cleanup to have its own timeout, register a DeferCleanup that accepts a SpecContext and decorate it with NodeTimeout so cleanup runs with a non-cancelled context. [1][2]
Sources:
- Ginkgo docs: DeferCleanup/SpecContext behavior and examples. [1]
- Kubernetes e2e blog (examples & warning that BeforeEach ctx is cancelled when callback returns). [2]
🌐 Web query:
sudo -n flag non-interactive password behavior
💡 Result:
Short answer
- sudo -n (or --non-interactive) tells sudo to never prompt the user for input. If the command requires a password, sudo prints an error and exits instead of prompting. [1]
Behavior and exit indication
- With -n, sudo will "avoid prompting the user for input of any kind. If a password is required for the command to run, sudo will display an error message and exit." You can detect this by checking sudo's stderr (message like "sudo: a password is required") and the exit status. Example usages show sudo returning a non‑zero exit (commonly 1) when -n causes failure. [1][2]
Related sudoers option (optional)
- Starting in newer sudo versions there is a sudoers flag noninteractive_auth which, when enabled, allows authentication methods that do not require user interaction to be attempted even when -n is used. (If that flag is off, sudo will not attempt authentication in non‑interactive mode.) This is available in sudo ≥ 1.9.10. [3]
Sources
- sudo manual (–n/--non-interactive). [1]
- Example discussion showing error message and exit behaviour. [2]
- sudoers noninteractive_auth description (sudo ≥ 1.9.10). [3]
🏁 Script executed:
#!/bin/bash
# Check the test structure and file context
cd /tmp/verified_codebase 2>/dev/null || cd .
# Look for the test file
find . -name "provider_podman.go" -type f 2>/dev/null | head -5
# If found, check for parallel test markers and overall structure
if [ -f "e2e/tests/up/provider_podman.go" ]; then
echo "=== File found, checking test structure ==="
head -80 e2e/tests/up/provider_podman.go | cat -n
fiRepository: skevetter/devpod
Length of output: 2765
🏁 Script executed:
#!/bin/bash
# Check if this test suite uses parallel execution
rg -A5 "ginkgo\.Describe.*up-provider-podman" . 2>/dev/null || echo "Pattern not found"
# Look for parallelization settings
rg "ParallelNode|RunSpecs.*ParallelProcess" . 2>/dev/null || echo "No explicit parallel markers found"
# Check the broader test execution patterns
rg -B3 -A3 "exec\.Command" e2e/tests/up/ 2>/dev/null | head -40Repository: skevetter/devpod
Length of output: 2994
Fix parallel test flakiness and sudo hangs in rootfull podman wrapper setup.
Lines 42–65 create a wrapper at a fixed initialDir/bin/podman-rootful path shared across parallel test runs, invoke sudo without -n (blocking indefinitely if a password is required), and use exec.Command instead of exec.CommandContext (inconsistent with codebase patterns). Replace with a per-test temp bin directory, sudo -n, and exec.CommandContext to align with the kubernetes tests in this suite.
🔧 Suggested fix
ginkgo.BeforeEach(func(ctx context.Context) {
- wrapper, err := os.Create(initialDir + "/bin/podman-rootful")
+ tempBinDir, err := os.MkdirTemp("", "devpod-podman-rootful-*")
+ framework.ExpectNoError(err)
+ ginkgo.DeferCleanup(func() {
+ _ = os.RemoveAll(tempBinDir)
+ })
+
+ wrapperPath := tempBinDir + "/podman-rootful"
+ wrapper, err := os.Create(wrapperPath)
framework.ExpectNoError(err)
- defer func() { _ = wrapper.Close() }()
-
- _, err = wrapper.WriteString("#!/bin/sh\nsudo podman \"$@\"\n")
+ _, err = wrapper.WriteString("#!/bin/sh\nsudo -n podman \"$@\"\n")
framework.ExpectNoError(err)
err = wrapper.Close()
framework.ExpectNoError(err)
- err = os.Chmod(initialDir+"/bin/podman-rootful", 0755)
+ err = os.Chmod(wrapperPath, 0755)
framework.ExpectNoError(err)
- err = exec.Command(initialDir+"/bin/podman-rootful", "ps").Run()
+ err = exec.CommandContext(ctx, wrapperPath, "ps").Run()
framework.ExpectNoError(err)
- ginkgo.DeferCleanup(func() {
- _ = os.Remove(initialDir + "/bin/podman-rootful")
- })
-
- f, err = setupDockerProvider(initialDir+"/bin", initialDir+"/bin/podman-rootful")
+ f, err = setupDockerProvider(tempBinDir, wrapperPath)
framework.ExpectNoError(err)
})📝 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.
| ginkgo.BeforeEach(func(ctx context.Context) { | |
| wrapper, err := os.Create(initialDir + "/bin/podman-rootful") | |
| framework.ExpectNoError(err) | |
| defer func() { _ = wrapper.Close() }() | |
| _, err = wrapper.WriteString("#!/bin/sh\nsudo podman \"$@\"\n") | |
| framework.ExpectNoError(err) | |
| err = wrapper.Close() | |
| framework.ExpectNoError(err) | |
| err = os.Chmod(initialDir+"/bin/podman-rootful", 0755) | |
| framework.ExpectNoError(err) | |
| err = exec.Command(initialDir+"/bin/podman-rootful", "ps").Run() | |
| framework.ExpectNoError(err) | |
| ginkgo.DeferCleanup(func() { | |
| _ = os.Remove(initialDir + "/bin/podman-rootful") | |
| }) | |
| f, err = setupDockerProvider(initialDir+"/bin", initialDir+"/bin/podman-rootful") | |
| framework.ExpectNoError(err) | |
| ginkgo.BeforeEach(func(ctx context.Context) { | |
| tempBinDir, err := os.MkdirTemp("", "devpod-podman-rootful-*") | |
| framework.ExpectNoError(err) | |
| ginkgo.DeferCleanup(func() { | |
| _ = os.RemoveAll(tempBinDir) | |
| }) | |
| wrapperPath := tempBinDir + "/podman-rootful" | |
| wrapper, err := os.Create(wrapperPath) | |
| framework.ExpectNoError(err) | |
| _, err = wrapper.WriteString("#!/bin/sh\nsudo -n podman \"$@\"\n") | |
| framework.ExpectNoError(err) | |
| err = wrapper.Close() | |
| framework.ExpectNoError(err) | |
| err = os.Chmod(wrapperPath, 0755) | |
| framework.ExpectNoError(err) | |
| err = exec.CommandContext(ctx, wrapperPath, "ps").Run() | |
| framework.ExpectNoError(err) | |
| f, err = setupDockerProvider(tempBinDir, wrapperPath) | |
| framework.ExpectNoError(err) |
🤖 Prompt for AI Agents
In `@e2e/tests/up/provider_podman.go` around lines 42 - 65, The BeforeEach setup
creates a fixed wrapper at initialDir+"/bin/podman-rootful", calls sudo without
-n and uses exec.Command, causing parallel-test conflicts and potential
blocking; change it to create a per-test temp bin directory (use os.MkdirTemp or
similar) and write the wrapper there (unique name per test), update the wrapper
content to use "sudo -n podman \"$@\"" to avoid password prompts, replace
exec.Command(...) with exec.CommandContext(ctx, ...) when invoking the wrapper
(ps call) and when passing that temp bin path into
setupDockerProvider(initialBinDir, wrapperPath), and ensure cleanup removes the
temp bin directory in ginkgo.DeferCleanup.
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e/tests/up/git_repositories.go`:
- Around line 33-42: The test case in the ginkgo.It block uses the author's fork
"github.com/skevetter/devpod@pull/100/head"; update the repository reference
used in the call to f.DevPodUp to a stable official source (for example
"github.com/loft-sh/devpod@pull/100/head" or better a known branch or a
dedicated test repo/PR under loft-sh) so the test does not rely on a personal
fork; locate the anonymous ginkgo.It test and change the argument passed to
f.DevPodUp (and adjust any related cleanup/name variables if needed) to point to
the chosen official/branch reference.
♻️ Duplicate comments (1)
e2e/tests/up/up.go (1)
75-87: Temp directory is never removed.
framework.CreateTempDir()creates a directory, but only the files inside it are deleted via individualdeferstatements. The directory itself leaks across test runs.Suggested fix
// set env vars with file tmpDir, err := framework.CreateTempDir() framework.ExpectNoError(err) + defer func() { _ = os.RemoveAll(tmpDir) }() // create invalid env file invalidData := []byte("TEST VAR=" + value) workspaceEnvFileInvalid := filepath.Join(tmpDir, ".invalid") err = os.WriteFile( workspaceEnvFileInvalid, invalidData, 0o644) framework.ExpectNoError(err) - defer func() { _ = os.Remove(workspaceEnvFileInvalid) }()Note: With
os.RemoveAll(tmpDir)deferred, the individual file removal defers (lines 86, 99, 126) become redundant and can be removed.
🧹 Nitpick comments (2)
e2e/tests/up/provider_kubernetes.go (2)
44-54: Consider extracting kubectl polling to a helper function.The same kubectl polling pattern appears three times with only the expected count varying. Extracting this to a helper would reduce duplication and improve maintainability.
♻️ Suggested helper function
Add to the test file or framework:
func waitForPodCount(ctx context.Context, namespace string, expected int, description string) *framework.PodList { list := &framework.PodList{} gomega.Eventually(func() int { cmd := exec.CommandContext(ctx, "kubectl", "get", "pods", "-l", "devpod.sh/created=true", "-o", "json", "-n", namespace) stdout, err := cmd.Output() if err != nil { return -1 } if err := json.Unmarshal(stdout, list); err != nil { return -1 } return len(list.Items) }, 30*time.Second, 500*time.Millisecond).Should(gomega.Equal(expected), description) return list }Then use:
list := waitForPodCount(ctx, "devpod", 1, "Expect 1 pod") framework.ExpectEqual(len(list.Items[0].Spec.Containers), 1, "Expect 1 container")Also applies to: 67-78, 86-97
85-101: Minor inconsistency in assertions between first and second DevPodUp cycles.The first DevPodUp cycle (lines 55-56) validates container count and image, but the second cycle only verifies pod existence. If consistent validation is desired after restart, consider adding the same assertions here. Otherwise, this is acceptable if the goal is just to verify the workspace restarts correctly.
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e/tests/up/docker_wsl.go`:
- Around line 19-48: This Windows-specific suite must be gated to avoid running
on non-Windows CI: either add a Go build constraint `//go:build windows` at the
top of the file to compile the whole file only on Windows, or modify the
ginkgo.BeforeEach function to check runtime.GOOS and call ginkgo.Skip when not
"windows"; update the existing BeforeEach (the function that sets
originalDockerHost and DOCKER_HOST) to perform `if runtime.GOOS != "windows" {
ginkgo.Skip("windows-only test") }` before any Windows-specific setup so the
Describe/ginkgo test (and teardown in AfterEach that restores DOCKER_HOST) never
runs on non-Windows agents.
🧹 Nitpick comments (1)
e2e/tests/up/docker_wsl.go (1)
128-132: Clarify the path comparison logic.The expected value concatenates
"workspaces" + filepath.Base(tempDir)without a separator, which works becauseframework.CleanStringstrips all slashes from both the actual container path (/workspaces/basename) and the expected value. While functionally correct, this could be confusing for maintainers. Consider adding a brief comment explaining the slash-stripping behavior.Suggested clarification
containerWorkspaceFolder, _, err := f.ExecCommandCapture(ctx, []string{"ssh", "--command", "cat $HOME/container-workspace-folder.out", projectName}) framework.ExpectNoError(err) + // CleanString removes all slashes, so "/workspaces/foo" becomes "workspacesfoo" gomega.Expect(framework.CleanString(containerWorkspaceFolder)).To(gomega.Equal( framework.CleanString("workspaces" + filepath.Base(tempDir)), ))
| var _ = ginkgo.Describe("testing up command for windows", ginkgo.Label("up-docker-wsl"), func() { | ||
| var f *framework.Framework | ||
| var dockerHelper *docker.DockerHelper | ||
| var initialDir string | ||
| var originalDockerHost string | ||
| var err error | ||
|
|
||
| ginkgo.BeforeEach(func(ctx context.Context) { | ||
| if runtime.GOOS != "windows" { | ||
| ginkgo.Skip("WSL tests only run on Windows") | ||
| } | ||
| ginkgo.BeforeEach(func(ctx context.Context) { | ||
| initialDir, err = os.Getwd() | ||
| framework.ExpectNoError(err) | ||
|
|
||
| var err error | ||
| initialDir, err = os.Getwd() | ||
| framework.ExpectNoError(err) | ||
| dockerHelper = &docker.DockerHelper{DockerCommand: "docker", Log: log.Default} | ||
|
|
||
| dockerHelper = &docker.DockerHelper{DockerCommand: "docker", Log: log.Default} | ||
| originalDockerHost = os.Getenv("DOCKER_HOST") | ||
| if originalDockerHost == "" { | ||
| err = os.Setenv("DOCKER_HOST", "npipe:////./pipe/podman-machine-default") | ||
| framework.ExpectNoError(err) | ||
| } | ||
|
|
||
| originalDockerHost = os.Getenv("DOCKER_HOST") | ||
| err = os.Setenv("DOCKER_HOST", "tcp://localhost:2375") | ||
| framework.ExpectNoError(err) | ||
| f, err = setupDockerProvider(filepath.Join(initialDir, "bin"), "docker") | ||
| framework.ExpectNoError(err) | ||
| }) | ||
|
|
||
| f, err = setupDockerProvider(initialDir+"/bin", "docker") | ||
| framework.ExpectNoError(err) | ||
| ginkgo.AfterEach(func() { | ||
| if originalDockerHost == "" { | ||
| _ = os.Unsetenv("DOCKER_HOST") | ||
| } else { | ||
| _ = os.Setenv("DOCKER_HOST", originalDockerHost) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if OS guard was added to the file
echo "=== Checking first 10 lines for build constraint ==="
head -10 e2e/tests/up/docker_wsl.go
echo -e "\n=== Checking for runtime.GOOS skip or ginkgo.Skip ==="
rg -n "runtime.GOOS|ginkgo.Skip" e2e/tests/up/docker_wsl.go
echo -e "\n=== Full context around BeforeEach ==="
rg -B 5 -A 15 "BeforeEach" e2e/tests/up/docker_wsl.go | head -40Repository: skevetter/devpod
Length of output: 925
Add OS guard to prevent test failures on non-Windows CI platforms.
This Windows-specific test suite uses the Windows named pipe npipe:////./pipe/podman-machine-default but lacks an OS guard. It will fail when CI runs on ubuntu-latest. Add either a //go:build windows constraint at the top of the file or a runtime.GOOS check with ginkgo.Skip in BeforeEach to skip the test on non-Windows platforms.
🤖 Prompt for AI Agents
In `@e2e/tests/up/docker_wsl.go` around lines 19 - 48, This Windows-specific suite
must be gated to avoid running on non-Windows CI: either add a Go build
constraint `//go:build windows` at the top of the file to compile the whole file
only on Windows, or modify the ginkgo.BeforeEach function to check runtime.GOOS
and call ginkgo.Skip when not "windows"; update the existing BeforeEach (the
function that sets originalDockerHost and DOCKER_HOST) to perform `if
runtime.GOOS != "windows" { ginkgo.Skip("windows-only test") }` before any
Windows-specific setup so the Describe/ginkgo test (and teardown in AfterEach
that restores DOCKER_HOST) never runs on non-Windows agents.
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K skevetter@pm.me
Summary by CodeRabbit
Tests
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.