refactor(e2e): test suite to use ginkgo.Describe#569
Merged
Conversation
Signed-off-by: Samuel K <skevetter@pm.me>
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughReplaces per-package DevPodDescribe wrappers with direct ginkgo.Describe across e2e tests; converts many suites to Describe/It with context-aware It signatures and DeferCleanup; and substantially rewrites build tests to centralize Dockerfile reading, prebuild hash calculation (multi-arch), buildx workflows, and image inspection helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite as Test Suite
participant Parser as Dockerfile Parser
participant Buildx as buildx
participant Registry as Image Registry
participant K8s as Kubernetes
TestSuite->>Parser: prepareDockerfileContent -> parse Dockerfile
Parser-->>TestSuite: parsed config + contentToParse
TestSuite->>Buildx: calculate prebuild hash (multi-arch) & buildx build (platforms)
Buildx->>Registry: push images (prebuildRepoName / GetImageName)
Registry-->>TestSuite: image available
TestSuite->>K8s: validateKubernetesDeploymentWithoutDocker (deploy & inspect)
K8s-->>TestSuite: deployment status & image verification
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. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/tests/build/build.go (1)
157-170:⚠️ Potential issue | 🟠 MajorSame empty
modifiedDockerfileContentsissue exists here.Same pattern as the first test - if the final stage already has a name,
modifiedDockerfileContentswill be empty and parsing will fail.Proposed fix
_, modifiedDockerfileContents, err := dockerfile.EnsureFinalStageName( string(dockerfileContent), config.DockerfileDefaultTarget, ) framework.ExpectNoError(err) + if modifiedDockerfileContents == "" { + modifiedDockerfileContents = string(dockerfileContent) + } // do the build🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/build/build.go` around lines 157 - 170, EnsureFinalStageName can return an empty modifiedDockerfileContents when no change was needed, causing dockerfile.Parse to receive an empty string; update the logic around dockerfile.EnsureFinalStageName/dockerfile.Parse to use the original dockerfileContent when modifiedDockerfileContents is empty (i.e., set a contentToParse = modifiedDockerfileContents if non-empty else string(dockerfileContent)), then call dockerfile.Parse(contentToParse) and continue populating config.ImageBuildInfo with the parsed file; keep existing error checks around EnsureFinalStageName and Parse.
🧹 Nitpick comments (5)
e2e/tests/ide/ide.go (1)
31-34: Inconsistent context usage.Lines 31 and 34 use
context.Background()while the localctxvariable was already created on line 21 for the same purpose. Consider usingctxconsistently throughout the test for clarity.♻️ Suggested fix for consistency
- err = f.DevPodProviderUse(context.Background(), "docker") + err = f.DevPodProviderUse(ctx, "docker") framework.ExpectNoError(err) - ginkgo.DeferCleanup(f.DevPodWorkspaceDelete, context.Background(), tempDir) + ginkgo.DeferCleanup(f.DevPodWorkspaceDelete, ctx, tempDir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/ide/ide.go` around lines 31 - 34, The test uses context.Background() in calls to DevPodProviderUse and ginkgo.DeferCleanup(DevPodWorkspaceDelete) while a local ctx variable was created earlier; replace those context.Background() usages with the existing ctx variable so DevPodProviderUse(ctx, "docker") and ginkgo.DeferCleanup(f.DevPodWorkspaceDelete, ctx, tempDir) use the same context for consistency and cancellation propagation.e2e/tests/build/build.go (3)
307-311: Useginkgo.Skipinstead ofreturnfor proper test reporting.Using
returncauses the test to silently pass on Windows rather than being marked as skipped in test reports. Useginkgo.Skipto properly communicate that the test was intentionally skipped.Proposed fix
ginkgo.It("build kubernetes dockerless", func() { // skip windows for now if runtime.GOOS == osWindows { - return + ginkgo.Skip("kubernetes dockerless tests not supported on Windows") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/build/build.go` around lines 307 - 311, The test body for ginkgo.It("build kubernetes dockerless") currently uses a plain return when runtime.GOOS == osWindows which makes the test silently pass on Windows; replace that early-return with a ginkgo.Skip call (e.g., ginkgo.Skip("skipping on windows")) so the test is reported as skipped. Update the conditional that checks runtime.GOOS == osWindows inside the ginkgo.It block (the "build kubernetes dockerless" test) to call ginkgo.Skip instead of returning.
362-368: Useginkgo.Skipin helper function as well.Same issue - using
returnin the helper silently passes the calling test instead of marking it as skipped.Proposed fix
func validateKubernetesDeploymentWithoutDocker( initialDir string, action func(context.Context, *framework.Framework, string) error, ) { if runtime.GOOS == osWindows { - return + ginkgo.Skip("kubernetes dockerless tests not supported on Windows") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/build/build.go` around lines 362 - 368, The helper validateKubernetesDeploymentWithoutDocker currently returns when runtime.GOOS == osWindows which makes callers silently succeed; replace that early return with a call to ginkgo.Skip (e.g., ginkgo.Skip("skipping on Windows")) so the test framework marks the test as skipped, and ensure the ginkgo package is imported in the file; update the function validateKubernetesDeploymentWithoutDocker accordingly.
124-126: Remove redundantInspectImagecall.The image is inspected on line 124, the result is discarded, and then immediately inspected again on line 126. Remove the first call.
Proposed fix
framework.ExpectNoError(err) - _, err = dockerHelper.InspectImage(ctx, prebuildRepoName+":"+prebuildHash, false) - framework.ExpectNoError(err) details, err := dockerHelper.InspectImage(ctx, prebuildRepoName+":"+prebuildHash, false) framework.ExpectNoError(err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/build/build.go` around lines 124 - 126, Remove the redundant dockerHelper.InspectImage invocation: the call that assigns to the blank identifier and its ExpectNoError (the first InspectImage using "_, err = dockerHelper.InspectImage(ctx, prebuildRepoName+":"+prebuildHash, false)") should be removed so you only call dockerHelper.InspectImage once (the call that assigns to details, err). Ensure framework.ExpectNoError(err) remains applied to the actual call that returns details.e2e/tests/context/context.go (1)
12-19: Move runtime setup out of theDescribebody.
os.Getwd()andframework.ExpectNoError(err)now run while Ginkgo is constructing the spec tree, before a spec is active. Put this in a setup node so failures are reported as spec failures instead of suite-construction errors.♻️ Suggested structure
func() { - ctx := context.Background() - initialDir, err := os.Getwd() - framework.ExpectNoError(err) + ctx := context.Background() + var initialDir string + + ginkgo.BeforeAll(func() { + var err error + initialDir, err = os.Getwd() + framework.ExpectNoError(err) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/context/context.go` around lines 12 - 19, The runtime initialization (calls to os.Getwd() and framework.ExpectNoError(err) that create ctx and initialDir) must be moved out of the immediate ginkgo.Describe body into a Ginkgo setup node (e.g., ginkgo.BeforeAll or ginkgo.BeforeEach) so failures are reported as spec failures; create package-scoped vars for ctx and initialDir, then in a BeforeAll hook call ctx = context.Background(), initialDir, err := os.Getwd(); call framework.ExpectNoError(err) inside that hook and remove the original os.Getwd()/ExpectNoError from the Describe constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/build/build.go`:
- Around line 265-269: The call to dockerfile.EnsureFinalStageName can return an
empty modifiedDockerfileContents which is currently not handled; update the code
around dockerfile.EnsureFinalStageName (the variables dockerfileContent,
modifiedDockerfileContents) to check for an empty modifiedDockerfileContents
after the call and either (a) fall back to the original
string(dockerfileContent) when modifiedDockerfileContents == "" or (b)
explicitly fail the test with a clear error (e.g.,
framework.ExpectTrue(len(modifiedDockerfileContents) > 0) or a Fatalf) so the
test doesn't proceed with an empty Dockerfile; keep the error check
(framework.ExpectNoError(err)) and add this post-call validation.
- Around line 291-300: The prebuild hash is hardcoded to linux/amd64 by calling
config.CalculatePrebuildHash with Platform:"linux/amd64" and
Architecture:"amd64", which breaks on non-amd64 builders; update the call in
build.go to derive platform and architecture from the runtime (e.g. use
runtime.GOARCH and construct the platform string like "linux/"+runtime.GOARCH,
matching how skip-push handles it) and pass those values into PrebuildHashParams
(Platform and Architecture) so the computed hash matches the actual built image
architecture.
- Around line 73-96: Ensure you handle the case where
dockerfile.EnsureFinalStageName returns an empty modifiedDockerfileContents
(meaning final stage already had an AS clause) before calling dockerfile.Parse:
if modifiedDockerfileContents is empty, fall back to using the original
dockerfileContent (the variable passed into EnsureFinalStageName) when invoking
dockerfile.Parse so that parsing and subsequent use in config.ImageBuildInfo
(the Dockerfile assigned to info) use the correct non-empty content.
In `@e2e/tests/integration/integration.go`:
- Around line 85-114: The tests create shared provider/workspace state across
multiple It blocks causing flaky cleanup; refactor so setup/teardown are atomic:
either combine the four It cases into a single end-to-end test that calls
framework.NewDefaultFramework(...), DevPodProviderAdd, DevPodUp, DevPodSSH and
DevPodWorkspaceDelete in sequence (and removes the ssh provider with
DevPodProviderDelete at the end), or move provider/workspace creation into a
suite-level BeforeEach and ensure guaranteed teardown in AfterEach/AfterSuite
that calls DevPodWorkspaceDelete and DevPodProviderDelete to always remove "ssh"
even on failures.
In `@e2e/tests/machineprovider/machineprovider.go`:
- Line 58: The test schedules f.DevPodWorkspaceDelete twice (once via explicit
call and once via ginkgo.DeferCleanup, and in another spec it queues it twice),
which can cause a failing "not found" delete to mask real failures; remove the
duplicate deletion paths by deleting the explicit calls to
f.DevPodWorkspaceDelete and ensure each tempDir is queued exactly once with
ginkgo.DeferCleanup (i.e., keep a single
ginkgo.DeferCleanup(f.DevPodWorkspaceDelete, context.Background(), tempDir) and
remove any additional calls or duplicate DeferCleanup invocations for the same
tempDir).
- Around line 125-135: Remove the unused temporary directory creation and
cleanup: delete the os.MkdirTemp call that assigns tempDirLocation, the
ginkgo.DeferCleanup block that calls os.RemoveAll(tempDirLocation), and any
reference to tempDirLocation; leave the provider setup calls
(f.DevPodProviderDelete, f.DevPodProviderAdd using filepath.Join(tempDir,
"provider.yaml")) intact. This removes the unnecessary tempDirLocation variable
and its cleanup around the inactivity timeout test without affecting provider
creation logic.
In `@e2e/tests/ssh/ssh.go`:
- Around line 132-139: The spawned background process created via
exec.CommandContext(ctx, ...) and started with serverCmd.Start() is never reaped
because ctx is a non-cancelable background context; fix by making the command
use a cancelable context (e.g., ctx, cancel := context.WithCancel(...) or
context.WithTimeout(...)) and defer cancel() in the test, then after Start()
ensure you reap the child by calling serverCmd.Wait() (best in a goroutine to
avoid blocking the test) or ensure Wait() is called in a cleanup path so the
process is reaped when the test finishes; reference serverCmd,
exec.CommandContext, Start(), Wait(), and ctx to locate where to change the
context creation and add the deferred cancel + Wait goroutine.
---
Outside diff comments:
In `@e2e/tests/build/build.go`:
- Around line 157-170: EnsureFinalStageName can return an empty
modifiedDockerfileContents when no change was needed, causing dockerfile.Parse
to receive an empty string; update the logic around
dockerfile.EnsureFinalStageName/dockerfile.Parse to use the original
dockerfileContent when modifiedDockerfileContents is empty (i.e., set a
contentToParse = modifiedDockerfileContents if non-empty else
string(dockerfileContent)), then call dockerfile.Parse(contentToParse) and
continue populating config.ImageBuildInfo with the parsed file; keep existing
error checks around EnsureFinalStageName and Parse.
---
Nitpick comments:
In `@e2e/tests/build/build.go`:
- Around line 307-311: The test body for ginkgo.It("build kubernetes
dockerless") currently uses a plain return when runtime.GOOS == osWindows which
makes the test silently pass on Windows; replace that early-return with a
ginkgo.Skip call (e.g., ginkgo.Skip("skipping on windows")) so the test is
reported as skipped. Update the conditional that checks runtime.GOOS ==
osWindows inside the ginkgo.It block (the "build kubernetes dockerless" test) to
call ginkgo.Skip instead of returning.
- Around line 362-368: The helper validateKubernetesDeploymentWithoutDocker
currently returns when runtime.GOOS == osWindows which makes callers silently
succeed; replace that early return with a call to ginkgo.Skip (e.g.,
ginkgo.Skip("skipping on Windows")) so the test framework marks the test as
skipped, and ensure the ginkgo package is imported in the file; update the
function validateKubernetesDeploymentWithoutDocker accordingly.
- Around line 124-126: Remove the redundant dockerHelper.InspectImage
invocation: the call that assigns to the blank identifier and its ExpectNoError
(the first InspectImage using "_, err = dockerHelper.InspectImage(ctx,
prebuildRepoName+":"+prebuildHash, false)") should be removed so you only call
dockerHelper.InspectImage once (the call that assigns to details, err). Ensure
framework.ExpectNoError(err) remains applied to the actual call that returns
details.
In `@e2e/tests/context/context.go`:
- Around line 12-19: The runtime initialization (calls to os.Getwd() and
framework.ExpectNoError(err) that create ctx and initialDir) must be moved out
of the immediate ginkgo.Describe body into a Ginkgo setup node (e.g.,
ginkgo.BeforeAll or ginkgo.BeforeEach) so failures are reported as spec
failures; create package-scoped vars for ctx and initialDir, then in a BeforeAll
hook call ctx = context.Background(), initialDir, err := os.Getwd(); call
framework.ExpectNoError(err) inside that hook and remove the original
os.Getwd()/ExpectNoError from the Describe constructor.
In `@e2e/tests/ide/ide.go`:
- Around line 31-34: The test uses context.Background() in calls to
DevPodProviderUse and ginkgo.DeferCleanup(DevPodWorkspaceDelete) while a local
ctx variable was created earlier; replace those context.Background() usages with
the existing ctx variable so DevPodProviderUse(ctx, "docker") and
ginkgo.DeferCleanup(f.DevPodWorkspaceDelete, ctx, tempDir) use the same context
for consistency and cancellation propagation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63f6e5f0-c57e-493e-84fa-7cf8ab2185a8
📒 Files selected for processing (18)
e2e/tests/build/build.goe2e/tests/build/framework.goe2e/tests/context/context.goe2e/tests/context/framework.goe2e/tests/ide/framework.goe2e/tests/ide/ide.goe2e/tests/integration/framework.goe2e/tests/integration/integration.goe2e/tests/machine/create.goe2e/tests/machine/delete.goe2e/tests/machine/framework.goe2e/tests/machineprovider/framework.goe2e/tests/machineprovider/machineprovider.goe2e/tests/provider/framework.goe2e/tests/provider/provider.goe2e/tests/ssh/framework.goe2e/tests/ssh/ssh.goe2e/tests/upgrade/upgrade.go
💤 Files with no reviewable changes (8)
- e2e/tests/ide/framework.go
- e2e/tests/provider/framework.go
- e2e/tests/machineprovider/framework.go
- e2e/tests/build/framework.go
- e2e/tests/context/framework.go
- e2e/tests/integration/framework.go
- e2e/tests/machine/framework.go
- e2e/tests/ssh/framework.go
Comment on lines
+26
to
+83
| ginkgo.It("should generate ssh keypairs", func(ctx context.Context) { | ||
| sshDir := os.Getenv("HOME") + "/.ssh" | ||
| if _, err := os.Stat(sshDir); os.IsNotExist(err) { | ||
| err = os.MkdirAll(sshDir, 0o700) | ||
| framework.ExpectNoError(err) | ||
| }) | ||
|
|
||
| ginkgo.It("should generate ssh keypairs", func(ctx context.Context) { | ||
| sshDir := os.Getenv("HOME") + "/.ssh" | ||
| if _, err := os.Stat(sshDir); os.IsNotExist(err) { | ||
| err = os.MkdirAll(sshDir, 0o700) | ||
| framework.ExpectNoError(err) | ||
| } | ||
|
|
||
| homeDir := os.Getenv("HOME") | ||
| sshKeyPath := filepath.Join(homeDir, ".ssh", "id_rsa") | ||
| sshPubKeyPath := filepath.Join(homeDir, ".ssh", "id_rsa.pub") | ||
|
|
||
| _, err := os.Stat(sshKeyPath) | ||
| if err != nil { | ||
| fmt.Println("generating ssh keys") | ||
| // #nosec G204 -- ssh-keygen with fixed arguments for test setup | ||
| cmd := exec.CommandContext( | ||
| ctx, | ||
| "ssh-keygen", | ||
| "-q", | ||
| "-t", | ||
| "rsa", | ||
| "-N", | ||
| "", | ||
| "-f", | ||
| sshKeyPath, | ||
| ) | ||
| err = cmd.Run() | ||
| framework.ExpectNoError(err) | ||
|
|
||
| // #nosec G204 -- ssh-keygen with fixed arguments for test setup | ||
| cmd = exec.CommandContext(ctx, "ssh-keygen", "-y", "-f", sshKeyPath) | ||
| output, err := cmd.Output() | ||
| framework.ExpectNoError(err) | ||
|
|
||
| err = os.WriteFile(sshPubKeyPath, output, 0o600) | ||
| framework.ExpectNoError(err) | ||
| } | ||
| } | ||
|
|
||
| homeDir := os.Getenv("HOME") | ||
| sshKeyPath := filepath.Join(homeDir, ".ssh", "id_rsa") | ||
| sshPubKeyPath := filepath.Join(homeDir, ".ssh", "id_rsa.pub") | ||
|
|
||
| _, err := os.Stat(sshKeyPath) | ||
| if err != nil { | ||
| ginkgo.GinkgoWriter.Println("generating ssh keys") | ||
| // #nosec G204 -- ssh-keygen with fixed arguments for test setup | ||
| cmd := exec.CommandContext(ctx, "ssh-keygen", "-y", "-f", sshKeyPath) | ||
| publicKey, err := cmd.Output() | ||
| cmd := exec.CommandContext( | ||
| ctx, | ||
| "ssh-keygen", | ||
| "-q", | ||
| "-t", | ||
| "rsa", | ||
| "-N", | ||
| "", | ||
| "-f", | ||
| sshKeyPath, | ||
| ) | ||
| err = cmd.Run() | ||
| framework.ExpectNoError(err) | ||
|
|
||
| authorizedKeysPath := filepath.Join(homeDir, ".ssh", "authorized_keys") | ||
| _, err = os.Stat(authorizedKeysPath) | ||
| if err != nil { | ||
| err = os.WriteFile(authorizedKeysPath, publicKey, 0o600) | ||
| framework.ExpectNoError(err) | ||
| } else { | ||
| f, err := os.OpenFile(os.Getenv("HOME")+"/.ssh/authorized_keys", | ||
| os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) | ||
| framework.ExpectNoError(err) | ||
|
|
||
| defer func() { _ = f.Close() }() | ||
| _, err = f.Write(publicKey) | ||
| framework.ExpectNoError(err) | ||
| } | ||
| }) | ||
|
|
||
| ginkgo.It("should add provider to devpod", func(ctx context.Context) { | ||
| f := framework.NewDefaultFramework(initialDir + "/bin") | ||
| // ensure we don't have the ssh provider present | ||
| err := f.DevPodProviderDelete(ctx, "ssh") | ||
| if err != nil { | ||
| fmt.Println("warning: " + err.Error()) | ||
| } | ||
|
|
||
| err = f.DevPodProviderAdd(ctx, "ssh", "-o", "HOST=localhost") | ||
| // #nosec G204 -- ssh-keygen with fixed arguments for test setup | ||
| cmd = exec.CommandContext(ctx, "ssh-keygen", "-y", "-f", sshKeyPath) | ||
| output, err := cmd.Output() | ||
| framework.ExpectNoError(err) | ||
| }) | ||
|
|
||
| ginkgo.It("should run devpod up", func(ctx context.Context) { | ||
| f := framework.NewDefaultFramework(initialDir + "/bin") | ||
| err := f.DevPodUp(ctx, "tests/integration/testdata/") | ||
| err = os.WriteFile(sshPubKeyPath, output, 0o600) | ||
| framework.ExpectNoError(err) | ||
| }) | ||
| } | ||
|
|
||
| // #nosec G204 -- ssh-keygen with fixed arguments for test setup | ||
| cmd := exec.CommandContext(ctx, "ssh-keygen", "-y", "-f", sshKeyPath) | ||
| publicKey, err := cmd.Output() | ||
| framework.ExpectNoError(err) | ||
|
|
||
| ginkgo.It("should run commands to workspace via ssh", func(ctx context.Context) { | ||
| f := framework.NewDefaultFramework(initialDir + "/bin") | ||
| out, err := f.DevPodSSH(ctx, "testdata", "echo test") | ||
| authorizedKeysPath := filepath.Join(homeDir, ".ssh", "authorized_keys") | ||
| _, err = os.Stat(authorizedKeysPath) | ||
| if err != nil { | ||
| err = os.WriteFile(authorizedKeysPath, publicKey, 0o600) | ||
| framework.ExpectNoError(err) | ||
| } else { | ||
| f, err := os.OpenFile(os.Getenv("HOME")+"/.ssh/authorized_keys", | ||
| os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) | ||
| framework.ExpectNoError(err) | ||
| framework.ExpectEqual(out, "test\n") | ||
| }) | ||
|
|
||
| ginkgo.It("should cleanup devpod workspace", func(ctx context.Context) { | ||
| f := framework.NewDefaultFramework(initialDir + "/bin") | ||
| err := f.DevPodWorkspaceDelete(ctx, "testdata") | ||
| defer func() { _ = f.Close() }() | ||
| _, err = f.Write(publicKey) | ||
| framework.ExpectNoError(err) | ||
| }) | ||
| }, | ||
| ) | ||
| }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Don't mutate the real $HOME/.ssh from this spec.
This now creates host-level id_rsa* files and appends to authorized_keys with no cleanup. Local runs become stateful, and CI/user accounts keep accumulating test artifacts. Use a temporary HOME/SSH dir for the suite, or snapshot and restore these files in cleanup.
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/tests/context/context.go (1)
55-58:⚠️ Potential issue | 🟠 MajorRestore
DEVPOD_CONTEXTbefore the deferred teardown runs.
os.Setenvmutates process-global state and this spec never puts it back. Because the delete cleanup was registered earlier, it currently runs withDEVPOD_CONTEXT=test-ctx-b-ide, and both delete errors are discarded, so a leaked active context can be hidden while later specs inherit the wrong default context. Save the previous value and restore or unset it via a laterginkgo.DeferCleanupso it runs first during teardown.Suggested fix
+ oldContext, hadOldContext := os.LookupEnv("DEVPOD_CONTEXT") err = os.Setenv("DEVPOD_CONTEXT", contextB) framework.ExpectNoError(err) + ginkgo.DeferCleanup(func() { + if hadOldContext { + framework.ExpectNoError(os.Setenv("DEVPOD_CONTEXT", oldContext)) + return + } + framework.ExpectNoError(os.Unsetenv("DEVPOD_CONTEXT")) + })Also applies to: 100-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/context/context.go` around lines 55 - 58, Save the previous DEVPOD_CONTEXT with os.LookupEnv before calling os.Setenv and register a ginkgo.DeferCleanup that restores (or unsets) that saved value (restore if present, unset if not) so the environment is returned to its original state; ensure this restore DeferCleanup is registered after the existing ginkgo.DeferCleanup that calls f.DevPodContextDelete(ctx, contextA) and f.DevPodContextDelete(ctx, contextB) so the restore runs first during teardown and the deletes observe the correct pre-test environment.
♻️ Duplicate comments (1)
e2e/tests/integration/integration.go (1)
28-83:⚠️ Potential issue | 🟠 MajorThis still mutates the caller's real
~/.sshstate.The spec writes
id_rsa*and appends toauthorized_keysunder$HOME, with no restoration. Local runs stay stateful, and repeated runs keep accumulating the test key. Please snapshot and restore these files in cleanup, or route the test through isolated key paths instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/integration/integration.go` around lines 28 - 83, The test currently mutates the caller's real ~/.ssh (sshDir, sshKeyPath, sshPubKeyPath, authorizedKeysPath) without restoring state; modify the test to avoid touching the real home by creating a temporary directory (os.MkdirTemp) for SSH files or snapshot existing files and restore them in a deferred cleanup: copy any existing id_rsa/id_rsa.pub/authorized_keys to temp backups, set HOME or point ssh-keygen/authorized_keys paths to the temp dir (use sshKeyPath/sshPubKeyPath/authorizedKeysPath variables), run the keygen and append operations against the temp files, and in a defer restore original files (or remove temp dir) and return original HOME/permissions; ensure all framework.ExpectNoError checks remain and that cleanup runs even on test failure.
🧹 Nitpick comments (1)
e2e/tests/ide/ide.go (1)
14-18: ConsiderBeforeAllfor one-time setup in anOrderedcontainer.Since this is an
Orderedcontainer with a singleItblock, usingginkgo.BeforeAllwould be more idiomatic and clearly signal thatinitialDircapture is a one-time setup operation. This also future-proofs the code if additionalItblocks are added.♻️ Suggested change
- ginkgo.BeforeEach(func() { + ginkgo.BeforeAll(func() { var err error initialDir, err = os.Getwd() framework.ExpectNoError(err) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/ide/ide.go` around lines 14 - 18, The setup currently captures initialDir inside ginkgo.BeforeEach using os.Getwd which runs before each spec; change this to ginkgo.BeforeAll so initialDir is captured once for the Ordered container (keep the same error handling via framework.ExpectNoError); locate the BeforeEach block around initialDir and replace it with a BeforeAll block that calls os.Getwd and assigns initialDir, preserving the existing error check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/build/build.go`:
- Around line 180-190: The call to config.CalculatePrebuildHash uses
modifiedDockerfileContents which may be empty and mismatches the earlier parsed
variable contentToParse; update the argument to pass contentToParse instead of
modifiedDockerfileContents so CalculatePrebuildHash receives the same Dockerfile
content used for parsing (change the DockerfileContent field in the
CalculatePrebuildHash call). Ensure you update the DockerfileContent parameter
in the CalculatePrebuildHash invocation near the block calling
CalculatePrebuildHash so it references contentToParse.
- Around line 103-126: The prebuild hash calls pass modifiedDockerfileContents
to DockerfileContent but parsing used contentToParse (and EnsureFinalStageName
may have left modifiedDockerfileContents empty); update both
CalculatePrebuildHash invocations (config.CalculatePrebuildHash with
config.PrebuildHashParams) to pass DockerfileContent: contentToParse instead of
modifiedDockerfileContents so the hash matches the actual Dockerfile used for
the build and subsequent dockerHelper.InspectImage checks.
In `@e2e/tests/context/context.go`:
- Around line 31-39: After each successful call to f.DevPodContextCreate (e.g.,
f.DevPodContextCreate(ctx, "test-context") and similar creates), immediately
register a cleanup using ginkgo.DeferCleanup that calls f.DevPodContextDelete
for the same context name so the delete runs even if subsequent ExpectNoError
fails; inside the deferred cleanup call f.DevPodContextDelete(ctx,
"<same-name>") and assert/handle any error (e.g., framework.ExpectNoError or
equivalent) so deletions are reported—do this for both the shown block and the
other create/delete pairs referenced (the second spec that creates
test-ctx-a-ide / test-ctx-b-ide).
In `@e2e/tests/integration/integration.go`:
- Around line 94-102: The cleanup callbacks for DeferCleanup are currently
ignoring errors from DevPodProviderDelete and DevPodWorkspaceDelete; update the
two ginkgo.DeferCleanup closures to capture the returned error and assert it
(e.g., call framework.ExpectNoError or similar) so teardown failures fail the
spec. Specifically modify the DeferCleanup that calls
f.DevPodProviderDelete(cleanupCtx, "ssh") and the one that calls
f.DevPodWorkspaceDelete(cleanupCtx, "testdata") to check their error return and
fail the test when non-nil.
In `@e2e/tests/machineprovider/machineprovider.go`:
- Around line 162-173: The loop polling for shutdown uses an exact comparison
status.State == "Stopped" which is inconsistent with earlier case-insensitive
checks and can hang if the CLI returns "STOPPED"; update the break condition in
the loop that calls f.DevPodStatus(ctx, tempDir, "--container-status=false") to
perform a case-insensitive comparison (e.g., use strings.EqualFold or normalize
strings.ToLower on status.State) so the loop exits when the state is any case
variation of "stopped".
- Around line 46-50: Replace uses of context.Background() passed to
ginkgo.DeferCleanup for long-running external calls (e.g.,
f.DevPodProviderDelete and f.DevPodWorkspaceDelete) with a cancellable timeout
context created via context.WithTimeout inside the spec (e.g., ctx, cancel :=
context.WithTimeout(context.Background(), 30*time.Second); defer cancel()). Pass
that ctx to f.DevPodProviderDelete and f.DevPodWorkspaceDelete in the
DeferCleanup calls and ensure cancel is deferred in the spec so cleanup can be
aborted if the command hangs; apply the same change to the other occurrences
noted (lines around the second spec and the 117-121/121 ranges).
In `@e2e/tests/ssh/ssh.go`:
- Line 39: The cleanup currently passes the test's spec ctx into
ginkgo.DeferCleanup (ginkgo.DeferCleanup(f.DevPodWorkspaceDelete, ctx,
tempDir)), which will be cancelled by the time Ginkgo runs cleanup; change it to
pass a cleanup-owned context by registering a closure that accepts the cleanup
context and calls f.DevPodWorkspaceDelete with that context and tempDir (i.e.,
replace the direct call with a closure that takes ctx and invokes
f.DevPodWorkspaceDelete(ctx, tempDir)); apply the same change for both
occurrences referenced.
---
Outside diff comments:
In `@e2e/tests/context/context.go`:
- Around line 55-58: Save the previous DEVPOD_CONTEXT with os.LookupEnv before
calling os.Setenv and register a ginkgo.DeferCleanup that restores (or unsets)
that saved value (restore if present, unset if not) so the environment is
returned to its original state; ensure this restore DeferCleanup is registered
after the existing ginkgo.DeferCleanup that calls f.DevPodContextDelete(ctx,
contextA) and f.DevPodContextDelete(ctx, contextB) so the restore runs first
during teardown and the deletes observe the correct pre-test environment.
---
Duplicate comments:
In `@e2e/tests/integration/integration.go`:
- Around line 28-83: The test currently mutates the caller's real ~/.ssh
(sshDir, sshKeyPath, sshPubKeyPath, authorizedKeysPath) without restoring state;
modify the test to avoid touching the real home by creating a temporary
directory (os.MkdirTemp) for SSH files or snapshot existing files and restore
them in a deferred cleanup: copy any existing id_rsa/id_rsa.pub/authorized_keys
to temp backups, set HOME or point ssh-keygen/authorized_keys paths to the temp
dir (use sshKeyPath/sshPubKeyPath/authorizedKeysPath variables), run the keygen
and append operations against the temp files, and in a defer restore original
files (or remove temp dir) and return original HOME/permissions; ensure all
framework.ExpectNoError checks remain and that cleanup runs even on test
failure.
---
Nitpick comments:
In `@e2e/tests/ide/ide.go`:
- Around line 14-18: The setup currently captures initialDir inside
ginkgo.BeforeEach using os.Getwd which runs before each spec; change this to
ginkgo.BeforeAll so initialDir is captured once for the Ordered container (keep
the same error handling via framework.ExpectNoError); locate the BeforeEach
block around initialDir and replace it with a BeforeAll block that calls
os.Getwd and assigns initialDir, preserving the existing error check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a989b2f-d3a5-4d8f-9bbe-98ec0cd05a7a
📒 Files selected for processing (6)
e2e/tests/build/build.goe2e/tests/context/context.goe2e/tests/ide/ide.goe2e/tests/integration/integration.goe2e/tests/machineprovider/machineprovider.goe2e/tests/ssh/ssh.go
Comment on lines
+46
to
+50
| ginkgo.DeferCleanup(func() { | ||
| err = f.DevPodProviderDelete(context.Background(), "docker123") | ||
| framework.ExpectNoError(err) | ||
|
|
||
| // check status | ||
| status, err := f.DevPodStatus(ctx, tempDir) | ||
| framework.ExpectNoError(err) | ||
| framework.ExpectEqual( | ||
| strings.ToUpper(status.State), | ||
| "RUNNING", | ||
| "workspace status did not match", | ||
| ) | ||
|
|
||
| // stop container | ||
| err = f.DevPodStop(ctx, tempDir) | ||
| framework.ExpectNoError(err) | ||
|
|
||
| // check status | ||
| status, err = f.DevPodStatus(ctx, tempDir) | ||
| framework.ExpectNoError(err) | ||
| framework.ExpectEqual( | ||
| strings.ToUpper(status.State), | ||
| "STOPPED", | ||
| "workspace status did not match", | ||
| ) | ||
|
|
||
| // wait for devpod workspace to come online (deadline: 30s) | ||
| err = f.DevPodUp(ctx, tempDir) | ||
| framework.ExpectNoError(err) | ||
|
|
||
| // check if ssh works as it should start the container | ||
| out, err := f.DevPodSSH( | ||
| ctx, | ||
| tempDir, | ||
| fmt.Sprintf("cat /workspaces/%s/test.txt", workspace.ID), | ||
| ) | ||
| framework.ExpectNoError(err) | ||
| framework.ExpectEqual( | ||
| strings.TrimSpace(out), | ||
| "Test123", | ||
| "workspace content does not match", | ||
| ) | ||
|
|
||
| // delete workspace | ||
| err = f.DevPodWorkspaceDelete(ctx, tempDir) | ||
| framework.ExpectNoError(err) | ||
| }, ginkgo.SpecTimeout(framework.GetTimeout())) | ||
|
|
||
| ginkgo.It("test devpod inactivity timeout", func(ctx context.Context) { | ||
| f := framework.NewDefaultFramework(initialDir + "/bin") | ||
|
|
||
| // copy test dir | ||
| tempDir, err := framework.CopyToTempDirWithoutChdir( | ||
| initialDir + "/tests/machineprovider/testdata/machineprovider2", | ||
| ) | ||
| }) | ||
| ginkgo.DeferCleanup(f.DevPodWorkspaceDelete, context.Background(), tempDir) |
There was a problem hiding this comment.
Bound cleanup commands with a timeout.
These DeferCleanup paths call external devpod commands with context.Background(). If one hangs, the spec timeout cannot cancel cleanup, and this ordered suite can stall indefinitely.
⏱️ Suggested fix
- ginkgo.DeferCleanup(func() {
- err = f.DevPodProviderDelete(context.Background(), "docker123")
- framework.ExpectNoError(err)
- })
- ginkgo.DeferCleanup(f.DevPodWorkspaceDelete, context.Background(), tempDir)
+ ginkgo.DeferCleanup(func() {
+ cleanupCtx, cancel := context.WithTimeout(context.Background(), framework.GetTimeout())
+ defer cancel()
+
+ err = f.DevPodProviderDelete(cleanupCtx, "docker123")
+ framework.ExpectNoError(err)
+ })
+ ginkgo.DeferCleanup(func() {
+ cleanupCtx, cancel := context.WithTimeout(context.Background(), framework.GetTimeout())
+ defer cancel()
+
+ err = f.DevPodWorkspaceDelete(cleanupCtx, tempDir)
+ framework.ExpectNoError(err)
+ })Apply the same pattern in the second spec as well.
Also applies to: 50-50, 117-121, 121-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/machineprovider/machineprovider.go` around lines 46 - 50, Replace
uses of context.Background() passed to ginkgo.DeferCleanup for long-running
external calls (e.g., f.DevPodProviderDelete and f.DevPodWorkspaceDelete) with a
cancellable timeout context created via context.WithTimeout inside the spec
(e.g., ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second);
defer cancel()). Pass that ctx to f.DevPodProviderDelete and
f.DevPodWorkspaceDelete in the DeferCleanup calls and ensure cancel is deferred
in the spec so cleanup can be aborted if the command hangs; apply the same
change to the other occurrences noted (lines around the second spec and the
117-121/121 ranges).
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
e2e/tests/context/context.go (1)
31-38:⚠️ Potential issue | 🟠 MajorQueue context deletion immediately after each create, using a cleanup-owned context.
The first spec only deletes on the happy path, and the IDE spec waits until after both creates to register cleanup while also closing over the spec
ctx. IfDevPodContextUseor the second create fails, the earlier context leaks into later ordered specs.Suggested fix
err = f.DevPodContextCreate(ctx, "test-context") framework.ExpectNoError(err) +ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + err := f.DevPodContextDelete(cleanupCtx, "test-context") + framework.ExpectNoError(err) +}) ... -err = f.DevPodContextDelete(ctx, "test-context") -framework.ExpectNoError(err)Apply the same pattern immediately after the
contextAandcontextBcreates.Also applies to: 48-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/context/context.go` around lines 31 - 38, The spec currently creates contexts (via DevPodContextCreate) but only registers deletion later, causing leaks if subsequent operations fail; after each call to DevPodContextCreate (for contextA and contextB) immediately register cleanup that calls DevPodContextDelete using a cleanup-owned context (do not close over the spec ctx) so the deletion runs regardless of later failures; update the sections that call DevPodContextCreate and DevPodContextUse to follow this pattern (register cleanup immediately after each create) so each created context is always queued for deletion.
🧹 Nitpick comments (2)
e2e/tests/build/build.go (2)
71-79: Extract the Dockerfile normalization flow into one helper.The
EnsureFinalStageName→ empty-content fallback →Parse→CalculatePrebuildHashsetup is duplicated across three specs. Given the empty-string contract onEnsureFinalStageName, keeping this inline in multiple places makes future fixes easy to miss.Also applies to: 95-109, 156-164, 171-185, 264-272, 289-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/build/build.go` around lines 71 - 79, Extract the Dockerfile normalization and prebuild-hash setup into a single helper function that encapsulates calling dockerfile.EnsureFinalStageName, applying the empty-string fallback, parsing the content via dockerfile.Parse, and invoking CalculatePrebuildHash; replace the duplicated inline sequences (the EnsureFinalStageName → contentToParse fallback → Parse → CalculatePrebuildHash flow) in the specs with calls to this helper. The helper should accept the raw dockerfileContent and any config constants (e.g., config.DockerfileDefaultTarget) and return the parsed result and prebuild hash or an error, and callers should use that single helper instead of repeating contentToParse/EnsureFinalStageName/Parse/CalculatePrebuildHash logic.
220-223: Align this spec with the context-aware pattern used in other specs.This spec uses
func()withcontext.Background()instead of accepting the Ginkgo-provided context like the other specs in the file. Accepting the context parameter will enable per-spec cancellation and timeouts for anyDevPodBuildoperations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/build/build.go` around lines 220 - 223, The test spec passed to ginkgo.It currently declares func() and creates its own context with context.Background(); change the spec to accept the Ginkgo-provided context parameter (i.e. use func(ctx context.Context)) and remove the manual context.Background() so that subsequent calls like DevPodBuild use the provided ctx for cancellation/timeouts consistent with other specs in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/build/build.go`:
- Around line 101-118: The test hashes and inspects the first prebuild image
using runtime.GOARCH, which causes the same architecture to be checked twice on
arm64 runners and may check a platform that wasn't built on other hosts; change
the first call to config.CalculatePrebuildHash to explicitly use "linux/amd64"
and Architecture "amd64" (so the two hashes inspected are for "linux/amd64" and
"linux/arm64"), then pass those tags to dockerHelper.InspectImage using
prebuildRepoName+":"+prebuildHash to verify both artifacts; update the
CalculatePrebuildHash calls that currently use runtime.GOARCH and the
corresponding Architecture values to use the explicit amd64/arm64 strings
instead.
In `@e2e/tests/machine/create.go`:
- Around line 31-37: After calling f.DevPodProviderAdd(ctx,
tempDir+"/mock-provider.yaml") register a cleanup that removes the provider
(e.g. call f.DevPodProviderRemove(ctx, "mock-provider") or the equivalent
teardown helper) immediately so the mock-provider cannot leak into other specs;
also change the provider switch call to use the spec's ctx (replace
DevPodProviderUse(context.Background(), "mock-provider") with
DevPodProviderUse(ctx, "mock-provider")) so the operation is bounded by the test
timeout/cancellation. Ensure the cleanup uses the same ctx and is registered
right after DevPodProviderAdd.
In `@e2e/tests/ssh/ssh.go`:
- Around line 32-41: Replace the plain defer call to
framework.CleanupTempDir(initialDir, tempDir) with a ginkgo.DeferCleanup
registration placed before the workspace cleanup registration so Ginkgo's LIFO
ordering ensures f.DevPodWorkspaceDelete(cleanupCtx, tempDir) runs first;
register like ginkgo.DeferCleanup(func(cleanupCtx context.Context){ if err :=
framework.CleanupTempDir(initialDir, tempDir); err != nil {
framework.ExpectNoError(err) }}), and ensure the existing ginkgo.DeferCleanup
wrapping f.DevPodWorkspaceDelete remains (so workspace deletion runs before
temp-dir cleanup) — apply the same change to both occurrences (the block using
framework.CleanupTempDir and the block using f.DevPodWorkspaceDelete).
---
Duplicate comments:
In `@e2e/tests/context/context.go`:
- Around line 31-38: The spec currently creates contexts (via
DevPodContextCreate) but only registers deletion later, causing leaks if
subsequent operations fail; after each call to DevPodContextCreate (for contextA
and contextB) immediately register cleanup that calls DevPodContextDelete using
a cleanup-owned context (do not close over the spec ctx) so the deletion runs
regardless of later failures; update the sections that call DevPodContextCreate
and DevPodContextUse to follow this pattern (register cleanup immediately after
each create) so each created context is always queued for deletion.
---
Nitpick comments:
In `@e2e/tests/build/build.go`:
- Around line 71-79: Extract the Dockerfile normalization and prebuild-hash
setup into a single helper function that encapsulates calling
dockerfile.EnsureFinalStageName, applying the empty-string fallback, parsing the
content via dockerfile.Parse, and invoking CalculatePrebuildHash; replace the
duplicated inline sequences (the EnsureFinalStageName → contentToParse fallback
→ Parse → CalculatePrebuildHash flow) in the specs with calls to this helper.
The helper should accept the raw dockerfileContent and any config constants
(e.g., config.DockerfileDefaultTarget) and return the parsed result and prebuild
hash or an error, and callers should use that single helper instead of repeating
contentToParse/EnsureFinalStageName/Parse/CalculatePrebuildHash logic.
- Around line 220-223: The test spec passed to ginkgo.It currently declares
func() and creates its own context with context.Background(); change the spec to
accept the Ginkgo-provided context parameter (i.e. use func(ctx
context.Context)) and remove the manual context.Background() so that subsequent
calls like DevPodBuild use the provided ctx for cancellation/timeouts consistent
with other specs in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4cc3bab-e5f3-466c-8279-bdfc7f24b03d
📒 Files selected for processing (6)
e2e/tests/build/build.goe2e/tests/context/context.goe2e/tests/machine/create.goe2e/tests/machine/delete.goe2e/tests/machineprovider/machineprovider.goe2e/tests/ssh/ssh.go
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/tests/machine/delete.go
Comment on lines
+32
to
+41
| defer framework.CleanupTempDir(initialDir, tempDir) | ||
|
|
||
| f := framework.NewDefaultFramework(initialDir + "/bin") | ||
| _ = f.DevPodProviderAdd(ctx, "docker") | ||
| err = f.DevPodProviderUse(ctx, "docker") | ||
| framework.ExpectNoError(err) | ||
|
|
||
| ginkgo.DeferCleanup(func(cleanupCtx context.Context) { | ||
| _ = f.DevPodWorkspaceDelete(cleanupCtx, tempDir) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the SSH test file
find . -name "ssh.go" -path "*/e2e/tests/*"Repository: skevetter/devpod
Length of output: 82
🏁 Script executed:
# Check the actual code at the specified lines
cat -n e2e/tests/ssh/ssh.go | sed -n '25,50p'Repository: skevetter/devpod
Length of output: 1138
🏁 Script executed:
# Also check the second location mentioned (115-124)
cat -n e2e/tests/ssh/ssh.go | sed -n '110,130p'Repository: skevetter/devpod
Length of output: 954
🏁 Script executed:
# Get overall context of the file structure
wc -l e2e/tests/ssh/ssh.go
head -50 e2e/tests/ssh/ssh.goRepository: skevetter/devpod
Length of output: 1401
🌐 Web query:
Ginkgo v2 DeferCleanup execution order vs Go defer
💡 Result:
Execution order: DeferCleanup vs Go defer
- Go
defer: deferred calls run when the surrounding function returns, in LIFO (last-in, first-out) order. [1] - Ginkgo v2
DeferCleanup: cleanup callbacks also run in LIFO order (useful for “tear down in reverse setup order”). [2]
So, order-wise, DeferCleanup behaves like Go defer (LIFO); the big difference is the scope / “when” they run.
When does DeferCleanup run (scope/timing)?
Ginkgo turns DeferCleanup into a dynamically-registered cleanup node whose execution time depends on where you call it:
- If called in
BeforeEach,JustBeforeEach,It,AfterEach,JustAfterEach→ runs when the spec completes (i.e., effectively like anAfterEach). [3] - If called in
BeforeAll/AfterAll(inside anOrderedcontainer) → runs when the ordered container completes (likeAfterAll). [3] - If called in
BeforeSuite/AfterSuite(including synchronized variants) → runs when the suite completes (likeAfterSuite). [3]
Also, when a spec is interrupted by a timeout, Ginkgo proceeds to run cleanup nodes (including DeferCleanup). [4]
Practical implication
- Use Go
deferfor cleanup that must happen at the end of a single helper/function. - Use Ginkgo
DeferCleanupfor cleanup that must happen at the end of a spec / ordered group / suite—even if the setup happened in a different function or node—while still getting LIFO teardown.
Sources
[1] Go blog: Defer, Panic, and Recover (LIFO order) https://go.dev/blog/defer-panic-and-recover [2] Kubernetes blog noting ginkgo.DeferCleanup runs in LIFO order https://www.kubernetes.dev/blog/2023/04/12/e2e-testing-best-practices-reloaded/ [3] Ginkgo v2 docs on DeferCleanup invocation timing by node context https://pkg.go.dev/github.com/onsi/ginkgo/v2 [4] Ginkgo docs: after timeout/interrupt, runs cleanup nodes incl. DeferCleanup https://onsi.github.io/ginkgo/
🌐 Web query:
Ginkgo v2 spec cleanup handlers LIFO ordering when function returns
💡 Result:
In Ginkgo v2, spec cleanup handlers registered via DeferCleanup(...):
- Run after the spec completes (i.e.,
DeferCleanupdynamically behaves like anAfterEachcleanup node when called fromBeforeEach/It/etc.). In other words, even if yourIt(...)function returns early, Ginkgo still runs theDeferCleanupcallbacks as part of spec teardown. [1][2] - Execute in LIFO order (last registered, first executed). This is one of the reasons
DeferCleanupis often preferred over manually-centralizedAfterEachteardown. [3]
Related nuance (often the source of confusion): Go’s defer (the language keyword) runs when the surrounding function returns—so defer inside It(func(){ ... }) runs when that It closure returns, before Ginkgo’s AfterEach/DeferCleanup teardown phase. [1][2]
Sources
[1] Ginkgo docs – “Cleaning up our Cleanup code: DeferCleanup” (DeferCleanup has identical runtime semantics to AfterEach) (onsi.github.io)
[2] pkg.go.dev docs for github.com/onsi/ginkgo/v2 – DeferCleanup invocation timing (“invoked when the spec completes”) (pkg.go.dev)
[3] Kubernetes Contributors blog – notes ginkgo.DeferCleanup executes in last-in-first-out (LIFO) order (kubernetes.dev)
Citations:
- 1: https://onsi.github.io/ginkgo/
- 2: https://pkg.go.dev/github.com/onsi/ginkgo/v2
- 3: https://www.kubernetes.dev/blog/2023/04/12/e2e-testing-best-practices-reloaded/?utm_source=openai
Defer statement removes tempDir before workspace cleanup completes.
Go's defer keyword executes when the It function returns, which happens before Ginkgo's DeferCleanup teardown phase runs. This means framework.CleanupTempDir(...) deletes the directory before f.DevPodWorkspaceDelete(cleanupCtx, tempDir) executes. Since the deletion error is discarded (_), a failed workspace delete silently leaks the workspace.
Convert defer to ginkgo.DeferCleanup and register it before the workspace cleanup so Ginkgo's LIFO ordering ensures the workspace deletes first. Capture and report the deletion error:
Suggested fix
- defer framework.CleanupTempDir(initialDir, tempDir)
+ ginkgo.DeferCleanup(framework.CleanupTempDir, initialDir, tempDir)
ginkgo.DeferCleanup(func(cleanupCtx context.Context) {
- _ = f.DevPodWorkspaceDelete(cleanupCtx, tempDir)
+ err := f.DevPodWorkspaceDelete(cleanupCtx, tempDir)
+ framework.ExpectNoError(err)
})Apply to both specs at lines 32-41 and 115-124.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/ssh/ssh.go` around lines 32 - 41, Replace the plain defer call to
framework.CleanupTempDir(initialDir, tempDir) with a ginkgo.DeferCleanup
registration placed before the workspace cleanup registration so Ginkgo's LIFO
ordering ensures f.DevPodWorkspaceDelete(cleanupCtx, tempDir) runs first;
register like ginkgo.DeferCleanup(func(cleanupCtx context.Context){ if err :=
framework.CleanupTempDir(initialDir, tempDir); err != nil {
framework.ExpectNoError(err) }}), and ensure the existing ginkgo.DeferCleanup
wrapping f.DevPodWorkspaceDelete remains (so workspace deletion runs before
temp-dir cleanup) — apply the same change to both occurrences (the block using
framework.CleanupTempDir and the block using f.DevPodWorkspaceDelete).
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
e2e/tests/context/context.go (2)
31-38:⚠️ Potential issue | 🟠 MajorRegister cleanup immediately after creating
test-context.If Line 34 fails,
test-contextleaks because deletion only happens on the happy path. Use a cleanup-owned context here and drop the inline delete.Suggested fix
err = f.DevPodContextCreate(ctx, "test-context") framework.ExpectNoError(err) + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + err := f.DevPodContextDelete(cleanupCtx, "test-context") + framework.ExpectNoError(err) + }) err = f.DevPodContextUse(ctx, "test-context") framework.ExpectNoError(err) - - err = f.DevPodContextDelete(ctx, "test-context") - framework.ExpectNoError(err) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/context/context.go` around lines 31 - 38, After creating "test-context" call the test cleanup registration immediately (so the context is deleted even if later steps fail) rather than deleting inline at the end; specifically, after DevPodContextCreate(ctx, "test-context") and its ExpectNoError, register a cleanup that calls DevPodContextDelete using a cleanup-owned context (e.g., via t.Cleanup or framework.RegisterCleanup) and then proceed to DevPodContextUse, removing the inline DevPodContextDelete call at the end.
51-53:⚠️ Potential issue | 🟠 MajorDon’t ignore context-delete failures in cleanup.
Both cleanup handlers discard
DevPodContextDeleteerrors, so stale contexts can silently bleed into later ordered specs or reruns.Suggested fix
ginkgo.DeferCleanup(func(cleanupCtx context.Context) { - _ = f.DevPodContextDelete(cleanupCtx, contextA) + err := f.DevPodContextDelete(cleanupCtx, contextA) + framework.ExpectNoError(err) }) @@ ginkgo.DeferCleanup(func(cleanupCtx context.Context) { - _ = f.DevPodContextDelete(cleanupCtx, contextB) + err := f.DevPodContextDelete(cleanupCtx, contextB) + framework.ExpectNoError(err) })Also applies to: 57-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/context/context.go` around lines 51 - 53, The cleanup currently swallows errors from DevPodContextDelete inside ginkgo.DeferCleanup; change the DeferCleanup handlers that call f.DevPodContextDelete (the one using contextA and the similar one for contextB) to capture the returned error and handle it (e.g., call ginkgo.Fail or t.Fatalf via the test context, or at minimum log the error via the test logger) instead of discarding it so failed deletions are visible and don’t leave stale contexts for later specs or reruns.e2e/tests/ssh/ssh.go (2)
38-44:⚠️ Potential issue | 🟠 MajorReverse these cleanup registrations.
Because
ginkgo.DeferCleanupruns in LIFO order, Lines 42-44 execute before Lines 38-40 today. That removestempDirbeforeDevPodWorkspaceDelete(cleanupCtx, tempDir)runs, and the delete error is silently dropped.Suggested fix
- ginkgo.DeferCleanup(func(cleanupCtx context.Context) { - _ = f.DevPodWorkspaceDelete(cleanupCtx, tempDir) - }) - ginkgo.DeferCleanup(func(cleanupCtx context.Context) { framework.CleanupTempDir(initialDir, tempDir) }) + + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + err := f.DevPodWorkspaceDelete(cleanupCtx, tempDir) + framework.ExpectNoError(err) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/ssh/ssh.go` around lines 38 - 44, The two ginkgo.DeferCleanup registrations are in the wrong order causing framework.CleanupTempDir(initialDir, tempDir) to run before f.DevPodWorkspaceDelete(cleanupCtx, tempDir), which removes tempDir prematurely; fix by reversing the registrations so the call to f.DevPodWorkspaceDelete(cleanupCtx, tempDir) is registered last (LIFO) and thus runs first, ensuring DevPodWorkspaceDelete executes while tempDir still exists—locate the ginkgo.DeferCleanup calls around f.DevPodWorkspaceDelete and framework.CleanupTempDir and swap their order (or register CleanupTempDir first and DevPodWorkspaceDelete second).
124-127:⚠️ Potential issue | 🟠 MajorSurface workspace teardown failures in the port-forward spec.
The combined cleanup hides
DevPodWorkspaceDeleteerrors, so a leaked workspace becomes silent shared state for the next spec.Suggested fix
ginkgo.DeferCleanup(func(cleanupCtx context.Context) { - _ = f.DevPodWorkspaceDelete(cleanupCtx, tempDir) + deleteErr := f.DevPodWorkspaceDelete(cleanupCtx, tempDir) framework.CleanupTempDir(initialDir, tempDir) + framework.ExpectNoError(deleteErr) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/ssh/ssh.go` around lines 124 - 127, The cleanup currently swallows errors from DevPodWorkspaceDelete by assigning its return to the blank identifier; change the deferred cleanup to capture the error from f.DevPodWorkspaceDelete(tempDir) and surface it instead of discarding it—e.g. check the returned error and fail the test or log it (use ginkgo.Fail or framework.Fail/Expect as appropriate) before calling framework.CleanupTempDir; update the anonymous function passed to ginkgo.DeferCleanup so DevPodWorkspaceDelete's error is inspected and reported rather than ignored.
🧹 Nitpick comments (3)
e2e/tests/build/build.go (3)
18-20: Isolate repository names per spec.
prebuildRepoNameis shared by the buildx and internal-buildkit checks. If a previous spec or earlier run already lefttest-repo:<hash>on the local daemon,InspectImagecan pass without proving the current path produced it. Deriving the repository from the spec ortempDirwould keep these E2E assertions self-contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/build/build.go` around lines 18 - 20, prebuildRepoName is a shared constant causing cross-spec collisions; change the tests to derive a unique repo name per spec/run (e.g., hash or sanitized basename of the spec or tempDir) instead of using the global prebuildRepoName constant — update the constant declaration in build.go and any uses (functions/methods that call InspectImage or reference prebuildRepoName) to call a helper like deriveRepoName(tempDir) so each test uses its own repository name and avoids false positives from leftover images.
42-60: Load the real devcontainer config before hashing.
pkg/devcontainer/config/prebuild.go:19-71includes the normalizedDevContainerConfigin the prebuild hash, but this helper reconstructs that config separately from the.devcontainer/devcontainer.jsonthatDevPodBuildactually reads. Any fixture drift here changes the expected tag independently of the system under test. Parsing the real file would keep the hash assertions aligned with the actual input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/build/build.go` around lines 42 - 60, The test helper getDevcontainerConfig currently reconstructs a DevContainerConfig by hand, causing hash drift versus what DevPodBuild actually reads; change getDevcontainerConfig to load and parse the real .devcontainer/devcontainer.json using the same loader used by the production code (the same parser/normalizer used in pkg/devcontainer/config/prebuild.go) and return that normalized DevContainerConfig so the prebuild hash matches DevPodBuild's input; ensure you call the identical parsing/normalization function/path and propagate any errors rather than hardcoding fields.
197-241: These compose specs need a post-build assertion.Both cases stop after
ExpectNoError(err), so they won’t catch regressions where the wrong service is built or the feature/context handling changes but the CLI still exits zero. Please inspect the resulting image, or assert on a service/feature-specific artifact after startup, so these tests validate behavior rather than process success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/build/build.go` around lines 197 - 241, After the DevPodBuild call in each ginkgo.It block, add a concrete post-build assertion instead of stopping at ExpectNoError(err): for the first test (which passes prebuildRepo) query the built image using the repository name (prebuildRepo) and assert the expected image/tag exists or contains an expected file/artifact; for the second test (docker-compose-features-context) start or inspect the built service (using tempDir as the workspace) and assert a feature-specific artifact or runtime behavior is present. Place these checks immediately after the DevPodBuild invocation (before the test returns) and use existing framework helpers (e.g., any image/registry inspect or workspace start/inspect helpers on f) and ensure cleanup via the existing DevPodWorkspaceDelete defer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/machine/create.go`:
- Around line 34-36: The cleanup currently swallows errors from
f.DevPodProviderDelete inside the ginkgo.DeferCleanup lambda; change the cleanup
to check the returned error and fail the spec on failure (e.g., call ginkgo.Fail
or use Gomega expectations) so a failed deletion of "mock-provider" does not get
ignored. Locate the ginkgo.DeferCleanup block that calls
f.DevPodProviderDelete(cleanupCtx, "mock-provider") and replace the ignored
assignment with explicit error handling that asserts the delete succeeded using
the test framework's failure/assertion API.
- Around line 42-43: The uuid.NewRandom() call currently ignores its error;
update the code that calls uuid.NewRandom() (the machineUUID variable creation)
to check the returned error and handle it explicitly (e.g., if err != nil then
fail the test with t.Fatalf or return the error), and only call
machineUUID.String() when err == nil so you don't proceed with uuid.Nil and risk
name collisions for machineName.
In `@e2e/tests/ssh/ssh.go`:
- Around line 34-35: The call to f.DevPodProviderAdd is currently discarding its
error (using `_ = ...`) which hides setup failures; change those calls to
capture and check the returned error (e.g., `err = f.DevPodProviderAdd(ctx,
"docker")`) and handle it the same way you handle errors from
f.DevPodProviderUse (return/fail the test or log and fail), applying this fix to
both occurrences of DevPodProviderAdd in the file so setup errors are propagated
rather than swallowed.
---
Duplicate comments:
In `@e2e/tests/context/context.go`:
- Around line 31-38: After creating "test-context" call the test cleanup
registration immediately (so the context is deleted even if later steps fail)
rather than deleting inline at the end; specifically, after
DevPodContextCreate(ctx, "test-context") and its ExpectNoError, register a
cleanup that calls DevPodContextDelete using a cleanup-owned context (e.g., via
t.Cleanup or framework.RegisterCleanup) and then proceed to DevPodContextUse,
removing the inline DevPodContextDelete call at the end.
- Around line 51-53: The cleanup currently swallows errors from
DevPodContextDelete inside ginkgo.DeferCleanup; change the DeferCleanup handlers
that call f.DevPodContextDelete (the one using contextA and the similar one for
contextB) to capture the returned error and handle it (e.g., call ginkgo.Fail or
t.Fatalf via the test context, or at minimum log the error via the test logger)
instead of discarding it so failed deletions are visible and don’t leave stale
contexts for later specs or reruns.
In `@e2e/tests/ssh/ssh.go`:
- Around line 38-44: The two ginkgo.DeferCleanup registrations are in the wrong
order causing framework.CleanupTempDir(initialDir, tempDir) to run before
f.DevPodWorkspaceDelete(cleanupCtx, tempDir), which removes tempDir prematurely;
fix by reversing the registrations so the call to
f.DevPodWorkspaceDelete(cleanupCtx, tempDir) is registered last (LIFO) and thus
runs first, ensuring DevPodWorkspaceDelete executes while tempDir still
exists—locate the ginkgo.DeferCleanup calls around f.DevPodWorkspaceDelete and
framework.CleanupTempDir and swap their order (or register CleanupTempDir first
and DevPodWorkspaceDelete second).
- Around line 124-127: The cleanup currently swallows errors from
DevPodWorkspaceDelete by assigning its return to the blank identifier; change
the deferred cleanup to capture the error from f.DevPodWorkspaceDelete(tempDir)
and surface it instead of discarding it—e.g. check the returned error and fail
the test or log it (use ginkgo.Fail or framework.Fail/Expect as appropriate)
before calling framework.CleanupTempDir; update the anonymous function passed to
ginkgo.DeferCleanup so DevPodWorkspaceDelete's error is inspected and reported
rather than ignored.
---
Nitpick comments:
In `@e2e/tests/build/build.go`:
- Around line 18-20: prebuildRepoName is a shared constant causing cross-spec
collisions; change the tests to derive a unique repo name per spec/run (e.g.,
hash or sanitized basename of the spec or tempDir) instead of using the global
prebuildRepoName constant — update the constant declaration in build.go and any
uses (functions/methods that call InspectImage or reference prebuildRepoName) to
call a helper like deriveRepoName(tempDir) so each test uses its own repository
name and avoids false positives from leftover images.
- Around line 42-60: The test helper getDevcontainerConfig currently
reconstructs a DevContainerConfig by hand, causing hash drift versus what
DevPodBuild actually reads; change getDevcontainerConfig to load and parse the
real .devcontainer/devcontainer.json using the same loader used by the
production code (the same parser/normalizer used in
pkg/devcontainer/config/prebuild.go) and return that normalized
DevContainerConfig so the prebuild hash matches DevPodBuild's input; ensure you
call the identical parsing/normalization function/path and propagate any errors
rather than hardcoding fields.
- Around line 197-241: After the DevPodBuild call in each ginkgo.It block, add a
concrete post-build assertion instead of stopping at ExpectNoError(err): for the
first test (which passes prebuildRepo) query the built image using the
repository name (prebuildRepo) and assert the expected image/tag exists or
contains an expected file/artifact; for the second test
(docker-compose-features-context) start or inspect the built service (using
tempDir as the workspace) and assert a feature-specific artifact or runtime
behavior is present. Place these checks immediately after the DevPodBuild
invocation (before the test returns) and use existing framework helpers (e.g.,
any image/registry inspect or workspace start/inspect helpers on f) and ensure
cleanup via the existing DevPodWorkspaceDelete defer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcd16829-e2fc-455c-b5c9-f95e2d71d5f7
📒 Files selected for processing (4)
e2e/tests/build/build.goe2e/tests/context/context.goe2e/tests/machine/create.goe2e/tests/ssh/ssh.go
Comment on lines
+34
to
+35
| _ = f.DevPodProviderAdd(ctx, "docker") | ||
| err = f.DevPodProviderUse(ctx, "docker") |
There was a problem hiding this comment.
Check DevPodProviderAdd instead of discarding it.
DevPodProviderAdd is already idempotent in the helper, so swallowing its return here only hides real setup failures.
Suggested fix
- _ = f.DevPodProviderAdd(ctx, "docker")
+ err = f.DevPodProviderAdd(ctx, "docker")
+ framework.ExpectNoError(err)
err = f.DevPodProviderUse(ctx, "docker")Apply the same change in both specs.
Also applies to: 120-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/ssh/ssh.go` around lines 34 - 35, The call to f.DevPodProviderAdd
is currently discarding its error (using `_ = ...`) which hides setup failures;
change those calls to capture and check the returned error (e.g., `err =
f.DevPodProviderAdd(ctx, "docker")`) and handle it the same way you handle
errors from f.DevPodProviderUse (return/fail the test or log and fail), applying
this fix to both occurrences of DevPodProviderAdd in the file so setup errors
are propagated rather than swallowed.
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
e2e/tests/context/context.go (2)
31-38:⚠️ Potential issue | 🟠 MajorRegister cleanup immediately after creating
test-context.If
DevPodContextUsefails after Line 31, Line 37 never runs and this ordered suite leaks the created context into later specs. This should useginkgo.DeferCleanupright after the successful create instead of relying on the happy-path delete.Proposed fix
err = f.DevPodContextCreate(ctx, "test-context") framework.ExpectNoError(err) + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + cleanupErr := f.DevPodContextDelete(cleanupCtx, "test-context") + framework.ExpectNoError(cleanupErr) + }) err = f.DevPodContextUse(ctx, "test-context") framework.ExpectNoError(err) - - err = f.DevPodContextDelete(ctx, "test-context") - framework.ExpectNoError(err) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/context/context.go` around lines 31 - 38, After successfully creating the context with f.DevPodContextCreate(ctx, "test-context"), immediately register a deferred cleanup using ginkgo.DeferCleanup to call f.DevPodContextDelete(ctx, "test-context") so the context is always removed even if f.DevPodContextUse(ctx, "test-context") or later steps fail; keep the explicit DevPodContextDelete call after use optional or remove it to avoid double-delete but ensure the cleanup uses the same ctx and "test-context" identifiers.
51-53:⚠️ Potential issue | 🟠 MajorDon't swallow cleanup failures for
contextA.The deferred cleanup at Line 51 currently ignores
DevPodContextDeleteerrors, so a leaked context can pass silently and destabilize reruns. Assert the delete result here the same way Line 58-Line 59 already does forcontextB.Proposed fix
ginkgo.DeferCleanup(func(cleanupCtx context.Context) { - _ = f.DevPodContextDelete(cleanupCtx, contextA) + cleanupErr := f.DevPodContextDelete(cleanupCtx, contextA) + framework.ExpectNoError(cleanupErr) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/context/context.go` around lines 51 - 53, The deferred cleanup for contextA currently discards the error from f.DevPodContextDelete; update the ginkgo.DeferCleanup anonymous func that calls f.DevPodContextDelete(cleanupCtx, contextA) to assert the deletion succeeded (same pattern used for contextB) instead of swallowing the return value — call f.DevPodContextDelete, capture its error result and use the test assertion (e.g., Expect(...).To(Succeed())) to fail the test on cleanup errors so leaked contexts are detected.e2e/tests/ssh/ssh.go (2)
35-37:⚠️ Potential issue | 🟠 MajorCheck
DevPodProviderAddinstead of discarding it.Both specs ignore provider-add failures, so a broken setup can be masked until
DevPodProviderUseor a later command fails against stale state. Mirror the existingExpectNoErrorhandling here as well.Also applies to: 118-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/ssh/ssh.go` around lines 35 - 37, The call to f.DevPodProviderAdd currently discards its error, masking failures; change both occurrences where DevPodProviderAdd is called (the earlier one near DevPodProviderUse and the later one paired with the second DevPodProviderUse) to capture the returned error and pass it to framework.ExpectNoError(err) just like DevPodProviderUse does, i.e., replace `_ = f.DevPodProviderAdd(ctx, "docker")` with `err = f.DevPodProviderAdd(ctx, "docker"); framework.ExpectNoError(err)` to ensure failures are surfaced.
39-42:⚠️ Potential issue | 🟠 MajorDon't swallow workspace cleanup failures in the first SSH spec.
This cleanup drops the
DevPodWorkspaceDeleteerror, so the spec can pass while leaving the workspace behind. The second spec already asserts that delete path; this one should too.🧹 Suggested fix
ginkgo.DeferCleanup(func(cleanupCtx context.Context) { - _ = f.DevPodWorkspaceDelete(cleanupCtx, tempDir) + err = f.DevPodWorkspaceDelete(cleanupCtx, tempDir) + framework.ExpectNoError(err) framework.CleanupTempDir(initialDir, tempDir) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/ssh/ssh.go` around lines 39 - 42, The cleanup currently swallows errors from f.DevPodWorkspaceDelete inside the ginkgo.DeferCleanup closure; change the closure to capture the error returned by f.DevPodWorkspaceDelete(cleanupCtx, tempDir) and assert it succeeds (e.g., using ginkgo.Expect/require or the test framework's assertion) instead of discarding it, then still call framework.CleanupTempDir(initialDir, tempDir) so both workspace deletion and temp dir cleanup run and any failure in DevPodWorkspaceDelete is reported.e2e/tests/integration/integration.go (1)
28-72:⚠️ Potential issue | 🟠 MajorDon't overwrite the host
~/.sshstate from this spec.This path creates
id_rsa*under the real$HOMEand replacesauthorized_keyswith a single generated key, with no restore step. Local runs become stateful and can clobber an existing SSH setup. Use a temporary HOME/.sshfor the test, or snapshot and restore these files in cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/integration/integration.go` around lines 28 - 72, The test currently writes SSH keys into the real user HOME (variables sshKeyPath, sshPubKeyPath, authorizedKeysPath) and can clobber a developer's SSH files; modify the setup to use an isolated temporary HOME/.ssh (e.g., create tmpDir via os.MkdirTemp and set os.Setenv("HOME", tmpDir) or build sshDir under tmpDir) or, if you must touch the real HOME, snapshot existing files (id_rsa, id_rsa.pub, authorized_keys) before writing and restore them in a deferred cleanup; update all references to sshKeyPath/sshPubKeyPath/authorizedKeysPath and ensure deferred removal of generated keys (or restoration of originals) so local state is not mutated after the test.e2e/tests/machine/machine.go (3)
34-35:⚠️ Potential issue | 🟠 MajorDo not ignore provider cleanup failures.
Silently swallowing the delete error here can leave shared provider state behind and make later e2e failures nondeterministic.
Suggested fix
ginkgo.DeferCleanup(func(cleanupCtx context.Context) { - _ = f.DevPodProviderDelete(cleanupCtx, "mock-provider") + err := f.DevPodProviderDelete(cleanupCtx, "mock-provider") + framework.ExpectNoError(err) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/machine/machine.go` around lines 34 - 35, The cleanup currently swallows the error from f.DevPodProviderDelete inside the ginkgo.DeferCleanup closure; update the closure to capture the returned error from f.DevPodProviderDelete(cleanupCtx, "mock-provider") and do not ignore it — if an error is returned, fail the test or call ginkgo.Fail/Expect(err).To help locate the change, modify the ginkgo.DeferCleanup(...) anonymous function that calls f.DevPodProviderDelete so it checks the error and propagates a test failure (or logs and fails) instead of assigning to the blank identifier.
42-43:⚠️ Potential issue | 🟡 MinorHandle
uuid.NewRandom()before derivingmachineName.Discarding the error here can fall through with
uuid.Nil, which turns rare entropy failures into misleading machine-name collisions.Suggested fix
- machineUUID, _ := uuid.NewRandom() + machineUUID, err := uuid.NewRandom() + framework.ExpectNoError(err) machineName := machineUUID.String()In github.com/google/uuid v1.6.0, what does uuid.NewRandom() return and when can it fail?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/machine/machine.go` around lines 42 - 43, The code ignores the error from uuid.NewRandom() and then uses machineUUID.String(), which can produce uuid.Nil on failure and cause misleading name collisions; update the call to uuid.NewRandom() to check the returned error (and optionally verify machineUUID != uuid.Nil) and handle failures explicitly—e.g., propagate/return the error or fail the test with a clear message—before assigning machineName so you never derive a name from a nil UUID.
64-70:⚠️ Potential issue | 🟠 MajorRegister provider cleanup here and use the spec context for the switch.
After the add succeeds, this spec mutates shared provider state but never tears it down, and
context.Background()lets a hung provider switch outlive the spec timeout.Suggested fix
ginkgo.By("Add mock provider") err = f.DevPodProviderAdd(ctx, tempDir+"/mock-provider.yaml") framework.ExpectNoError(err) + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + err := f.DevPodProviderDelete(cleanupCtx, "mock-provider") + framework.ExpectNoError(err) + }) ginkgo.By("Use mock provider") - err = f.DevPodProviderUse(context.Background(), "mock-provider") + err = f.DevPodProviderUse(ctx, "mock-provider") framework.ExpectNoError(err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/machine/machine.go` around lines 64 - 70, After calling f.DevPodProviderAdd(...) register a cleanup that removes the added provider (use the test framework's cleanup/teardown helper or call the provider delete/remove method) so the provider state is torn down after the spec, and when switching to the provider call f.DevPodProviderUse(...) with the spec context variable ctx (not context.Background()) so the switch respects the spec timeout; i.e., immediately after f.DevPodProviderAdd(ctx, ...) add a deferred/registered cleanup to remove the "mock-provider" and replace context.Background() with ctx when invoking f.DevPodProviderUse.
🧹 Nitpick comments (2)
e2e/tests/provider/provider.go (1)
12-23: Consider usingBeforeAllinstead ofBeforeEachforinitialDirinitialization.The
initialDirvariable captures the working directory, which remains constant throughout all tests. The relevant snippet fromcontext.go(lines 18-22) shows the same pattern usingginkgo.BeforeAll. Since this suite is markedginkgo.Orderedand the working directory doesn't change between tests,BeforeAllavoids redundantos.Getwd()calls.♻️ Suggested change
- ginkgo.BeforeEach(func() { + ginkgo.BeforeAll(func() { var err error initialDir, err = os.Getwd() framework.ExpectNoError(err) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/provider/provider.go` around lines 12 - 23, The test suite currently sets initialDir inside ginkgo.BeforeEach by calling os.Getwd(), which is unnecessary for an ordered suite; change the setup to use ginkgo.BeforeAll so initialDir is captured once: replace the ginkgo.BeforeEach block that assigns initialDir (and checks err with framework.ExpectNoError) with a ginkgo.BeforeAll block performing the same os.Getwd() call and error check, keeping the variable name initialDir and the existing error handling to mirror the pattern used in context.go.e2e/tests/machine/machine.go (1)
88-90: Assert the expected "not found" failure, not just any failure.
framework.ExpectError(err)makes this pass on unrelated delete regressions too. Checking the expected stderr or error substring will keep the spec focused on the non-existing-machine path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/machine/machine.go` around lines 88 - 90, Replace the broad failure check using framework.ExpectError(err) with an assertion that the delete failed specifically because the machine was not found: call f.DevPodMachineDelete([]string{machineName2}), assert err is non-nil, and verify the error string (or returned stderr) contains the expected "not found" substring (or the exact message your API returns) instead of accepting any error; update the assertion near the existing framework.ExpectError usage to check the error text for the not-found indicator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/integration/integration.go`:
- Around line 76-78: Replace the current catch-all warning for
DevPodProviderDelete errors with logic that treats only Kubernetes "not found"
as benign: call f.DevPodProviderDelete(ctx, "ssh"), and if err != nil then if
apierrors.IsNotFound(err) (from k8s.io/apimachinery/pkg/api/errors) write the
warning to ginkgo.GinkgoWriter, otherwise fail the spec immediately (e.g.
ginkgo.Fail(err.Error()) or use the test's fail helper) so unexpected
provider-delete errors stop the test; reference the DevPodProviderDelete call
and the ctx/"ssh" arguments when locating the code to change.
In `@e2e/tests/machine/machine.go`:
- Around line 46-50: The helpers DevPodMachineCreate and DevPodMachineDelete
currently call ExecCommand(context.Background(), ...) which detaches them from
the test cancellation; change these helpers to accept a context.Context
parameter (or use the existing test context field if framework struct exposes
it) and pass that context into ExecCommand (replace context.Background() with
the provided ctx) and update all call sites (e.g., the test that calls
f.DevPodMachineCreate and f.DevPodMachineDelete) to forward the test's context
(ctx or f.Context) so long-running create/delete operations are canceled when
the test context expires.
In `@e2e/tests/machineprovider/machineprovider.go`:
- Around line 111-113: The DevPodProviderDelete call currently swallows all
errors; change it to capture its error and only ignore the expected "provider
not found" case, otherwise fail test setup immediately. Specifically, replace
the discarded call to f.DevPodProviderDelete(ctx, "docker123") with error
handling that stores the result (err := f.DevPodProviderDelete(...)), checks for
the known not-found condition (by matching the provider-not-exist error string
or specific error type), and if the error is anything else call framework.Failf
or return the error before calling f.DevPodProviderAdd; leave the subsequent
DevPodProviderAdd and framework.ExpectNoError as-is.
- Around line 45-50: The cleanup closure currently calls f.DevPodWorkspaceDelete
and then f.DevPodProviderDelete inside one ginkgo.DeferCleanup, which aborts the
provider delete if workspace delete errors; update to register two separate
ginkgo.DeferCleanup callbacks: call ginkgo.DeferCleanup for
f.DevPodProviderDelete("docker123") first, then call ginkgo.DeferCleanup for
f.DevPodWorkspaceDelete(tempDir) second so LIFO ordering still deletes workspace
before provider but each cleanup runs independently even if one fails; locate
uses of ginkgo.DeferCleanup and the methods f.DevPodWorkspaceDelete and
f.DevPodProviderDelete (also apply the same change at the other occurrence
around lines 114-119).
In `@e2e/tests/provider/provider.go`:
- Around line 77-112: The bug is that the code shadows and cancels ctx then
continues to reuse that cancelled context for f.DevPodProviderUpdate and
f.DevPodProviderDelete; fix by creating new derived contexts (e.g., checkCtx,
checkCancel := context.WithDeadline(ctx, ...)) for each call to
f.DevPodProviderOptionsCheckNamespaceDescription, call checkCancel() after the
check completes, and ensure f.DevPodProviderUpdate and f.DevPodProviderDelete
are invoked with the original (uncancelled) ctx or a new dedicated context;
update references to cancel/ctx accordingly so you never reuse a cancelled
context when calling DevPodProviderUpdate or DevPodProviderDelete.
---
Duplicate comments:
In `@e2e/tests/context/context.go`:
- Around line 31-38: After successfully creating the context with
f.DevPodContextCreate(ctx, "test-context"), immediately register a deferred
cleanup using ginkgo.DeferCleanup to call f.DevPodContextDelete(ctx,
"test-context") so the context is always removed even if f.DevPodContextUse(ctx,
"test-context") or later steps fail; keep the explicit DevPodContextDelete call
after use optional or remove it to avoid double-delete but ensure the cleanup
uses the same ctx and "test-context" identifiers.
- Around line 51-53: The deferred cleanup for contextA currently discards the
error from f.DevPodContextDelete; update the ginkgo.DeferCleanup anonymous func
that calls f.DevPodContextDelete(cleanupCtx, contextA) to assert the deletion
succeeded (same pattern used for contextB) instead of swallowing the return
value — call f.DevPodContextDelete, capture its error result and use the test
assertion (e.g., Expect(...).To(Succeed())) to fail the test on cleanup errors
so leaked contexts are detected.
In `@e2e/tests/integration/integration.go`:
- Around line 28-72: The test currently writes SSH keys into the real user HOME
(variables sshKeyPath, sshPubKeyPath, authorizedKeysPath) and can clobber a
developer's SSH files; modify the setup to use an isolated temporary HOME/.ssh
(e.g., create tmpDir via os.MkdirTemp and set os.Setenv("HOME", tmpDir) or build
sshDir under tmpDir) or, if you must touch the real HOME, snapshot existing
files (id_rsa, id_rsa.pub, authorized_keys) before writing and restore them in a
deferred cleanup; update all references to
sshKeyPath/sshPubKeyPath/authorizedKeysPath and ensure deferred removal of
generated keys (or restoration of originals) so local state is not mutated after
the test.
In `@e2e/tests/machine/machine.go`:
- Around line 34-35: The cleanup currently swallows the error from
f.DevPodProviderDelete inside the ginkgo.DeferCleanup closure; update the
closure to capture the returned error from f.DevPodProviderDelete(cleanupCtx,
"mock-provider") and do not ignore it — if an error is returned, fail the test
or call ginkgo.Fail/Expect(err).To help locate the change, modify the
ginkgo.DeferCleanup(...) anonymous function that calls f.DevPodProviderDelete so
it checks the error and propagates a test failure (or logs and fails) instead of
assigning to the blank identifier.
- Around line 42-43: The code ignores the error from uuid.NewRandom() and then
uses machineUUID.String(), which can produce uuid.Nil on failure and cause
misleading name collisions; update the call to uuid.NewRandom() to check the
returned error (and optionally verify machineUUID != uuid.Nil) and handle
failures explicitly—e.g., propagate/return the error or fail the test with a
clear message—before assigning machineName so you never derive a name from a nil
UUID.
- Around line 64-70: After calling f.DevPodProviderAdd(...) register a cleanup
that removes the added provider (use the test framework's cleanup/teardown
helper or call the provider delete/remove method) so the provider state is torn
down after the spec, and when switching to the provider call
f.DevPodProviderUse(...) with the spec context variable ctx (not
context.Background()) so the switch respects the spec timeout; i.e., immediately
after f.DevPodProviderAdd(ctx, ...) add a deferred/registered cleanup to remove
the "mock-provider" and replace context.Background() with ctx when invoking
f.DevPodProviderUse.
In `@e2e/tests/ssh/ssh.go`:
- Around line 35-37: The call to f.DevPodProviderAdd currently discards its
error, masking failures; change both occurrences where DevPodProviderAdd is
called (the earlier one near DevPodProviderUse and the later one paired with the
second DevPodProviderUse) to capture the returned error and pass it to
framework.ExpectNoError(err) just like DevPodProviderUse does, i.e., replace `_
= f.DevPodProviderAdd(ctx, "docker")` with `err = f.DevPodProviderAdd(ctx,
"docker"); framework.ExpectNoError(err)` to ensure failures are surfaced.
- Around line 39-42: The cleanup currently swallows errors from
f.DevPodWorkspaceDelete inside the ginkgo.DeferCleanup closure; change the
closure to capture the error returned by f.DevPodWorkspaceDelete(cleanupCtx,
tempDir) and assert it succeeds (e.g., using ginkgo.Expect/require or the test
framework's assertion) instead of discarding it, then still call
framework.CleanupTempDir(initialDir, tempDir) so both workspace deletion and
temp dir cleanup run and any failure in DevPodWorkspaceDelete is reported.
---
Nitpick comments:
In `@e2e/tests/machine/machine.go`:
- Around line 88-90: Replace the broad failure check using
framework.ExpectError(err) with an assertion that the delete failed specifically
because the machine was not found: call
f.DevPodMachineDelete([]string{machineName2}), assert err is non-nil, and verify
the error string (or returned stderr) contains the expected "not found"
substring (or the exact message your API returns) instead of accepting any
error; update the assertion near the existing framework.ExpectError usage to
check the error text for the not-found indicator.
In `@e2e/tests/provider/provider.go`:
- Around line 12-23: The test suite currently sets initialDir inside
ginkgo.BeforeEach by calling os.Getwd(), which is unnecessary for an ordered
suite; change the setup to use ginkgo.BeforeAll so initialDir is captured once:
replace the ginkgo.BeforeEach block that assigns initialDir (and checks err with
framework.ExpectNoError) with a ginkgo.BeforeAll block performing the same
os.Getwd() call and error check, keeping the variable name initialDir and the
existing error handling to mirror the pattern used in context.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2be6faf0-f51e-44a4-a3ac-3658d90e85ec
📒 Files selected for processing (8)
e2e/tests/context/context.goe2e/tests/ide/ide.goe2e/tests/integration/integration.goe2e/tests/machine/delete.goe2e/tests/machine/machine.goe2e/tests/machineprovider/machineprovider.goe2e/tests/provider/provider.goe2e/tests/ssh/ssh.go
💤 Files with no reviewable changes (1)
- e2e/tests/machine/delete.go
Comment on lines
76
to
78
| if err := f.DevPodProviderDelete(ctx, "ssh"); err != nil { | ||
| ginkgo.GinkgoWriter.Println("warning: " + err.Error()) | ||
| } |
There was a problem hiding this comment.
Fail on unexpected provider-delete errors during setup.
If DevPodProviderDelete(ctx, "ssh") fails for anything other than the expected “not found” case, the spec keeps going with stale provider state and the later assertions become misleading. Filter only the benign absence case, otherwise fail here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/integration/integration.go` around lines 76 - 78, Replace the
current catch-all warning for DevPodProviderDelete errors with logic that treats
only Kubernetes "not found" as benign: call f.DevPodProviderDelete(ctx, "ssh"),
and if err != nil then if apierrors.IsNotFound(err) (from
k8s.io/apimachinery/pkg/api/errors) write the warning to ginkgo.GinkgoWriter,
otherwise fail the spec immediately (e.g. ginkgo.Fail(err.Error()) or use the
test's fail helper) so unexpected provider-delete errors stop the test;
reference the DevPodProviderDelete call and the ctx/"ssh" arguments when
locating the code to change.
Comment on lines
+46
to
+50
| err = f.DevPodMachineCreate([]string{machineName}) | ||
| framework.ExpectNoError(err) | ||
|
|
||
| ginkgo.By("Remove test machine") | ||
| err = f.DevPodMachineDelete([]string{machineName}) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'DevPodMachine(Create|Delete)\(' e2e/framework/command.go
rg -n -C2 'ExecCommand\(context\.Background\(' e2e/framework/command.goRepository: skevetter/devpod
Length of output: 1101
DevPodMachineCreate and DevPodMachineDelete bypass test context with hardcoded context.Background().
Both helpers in e2e/framework/command.go (lines 263 and 273) call ExecCommand(context.Background(), ...), which breaks the link to the test's cancellation context. A hung machine create or delete operation will not be canceled when the test context expires, blocking cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/machine/machine.go` around lines 46 - 50, The helpers
DevPodMachineCreate and DevPodMachineDelete currently call
ExecCommand(context.Background(), ...) which detaches them from the test
cancellation; change these helpers to accept a context.Context parameter (or use
the existing test context field if framework struct exposes it) and pass that
context into ExecCommand (replace context.Background() with the provided ctx)
and update all call sites (e.g., the test that calls f.DevPodMachineCreate and
f.DevPodMachineDelete) to forward the test's context (ctx or f.Context) so
long-running create/delete operations are canceled when the test context
expires.
Comment on lines
+45
to
50
| ginkgo.DeferCleanup(func(cleanupCtx context.Context) { | ||
| err = f.DevPodWorkspaceDelete(cleanupCtx, tempDir) | ||
| framework.ExpectNoError(err) | ||
| err = f.DevPodProviderDelete(cleanupCtx, "docker123") | ||
| framework.ExpectNoError(err) | ||
| }) |
There was a problem hiding this comment.
Split provider and workspace teardown into separate cleanup callbacks.
Right now a workspace-delete failure aborts the rest of the same cleanup closure, so DevPodProviderDelete(..., "docker123") never runs and the global provider can leak into later specs. Register provider cleanup first and workspace cleanup second so LIFO still deletes the workspace before the provider, but both cleanups always get a chance to run.
🧹 Suggested fix
- ginkgo.DeferCleanup(func(cleanupCtx context.Context) {
- err = f.DevPodWorkspaceDelete(cleanupCtx, tempDir)
- framework.ExpectNoError(err)
- err = f.DevPodProviderDelete(cleanupCtx, "docker123")
- framework.ExpectNoError(err)
- })
+ ginkgo.DeferCleanup(func(cleanupCtx context.Context) {
+ err = f.DevPodProviderDelete(cleanupCtx, "docker123")
+ framework.ExpectNoError(err)
+ })
+ ginkgo.DeferCleanup(func(cleanupCtx context.Context) {
+ err = f.DevPodWorkspaceDelete(cleanupCtx, tempDir)
+ framework.ExpectNoError(err)
+ })Also applies to: 114-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/machineprovider/machineprovider.go` around lines 45 - 50, The
cleanup closure currently calls f.DevPodWorkspaceDelete and then
f.DevPodProviderDelete inside one ginkgo.DeferCleanup, which aborts the provider
delete if workspace delete errors; update to register two separate
ginkgo.DeferCleanup callbacks: call ginkgo.DeferCleanup for
f.DevPodProviderDelete("docker123") first, then call ginkgo.DeferCleanup for
f.DevPodWorkspaceDelete(tempDir) second so LIFO ordering still deletes workspace
before provider but each cleanup runs independently even if one fails; locate
uses of ginkgo.DeferCleanup and the methods f.DevPodWorkspaceDelete and
f.DevPodProviderDelete (also apply the same change at the other occurrence
around lines 114-119).
Comment on lines
+111
to
+113
| _ = f.DevPodProviderDelete(ctx, "docker123") | ||
| err = f.DevPodProviderAdd(ctx, filepath.Join(tempDir, "provider.yaml")) | ||
| framework.ExpectNoError(err) |
There was a problem hiding this comment.
Don't ignore the pre-test provider delete failure.
If this delete fails for an unexpected reason, the subsequent DevPodProviderAdd error is only a downstream symptom. Ignore only the expected absence case; otherwise fail the setup immediately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/machineprovider/machineprovider.go` around lines 111 - 113, The
DevPodProviderDelete call currently swallows all errors; change it to capture
its error and only ignore the expected "provider not found" case, otherwise fail
test setup immediately. Specifically, replace the discarded call to
f.DevPodProviderDelete(ctx, "docker123") with error handling that stores the
result (err := f.DevPodProviderDelete(...)), checks for the known not-found
condition (by matching the provider-not-exist error string or specific error
type), and if the error is anything else call framework.Failf or return the
error before calling f.DevPodProviderAdd; leave the subsequent DevPodProviderAdd
and framework.ExpectNoError as-is.
Comment on lines
+77
to
+112
| ctx, cancel := context.WithDeadline( | ||
| ctx, | ||
| time.Now().Add(30*time.Second), | ||
| ) | ||
| err = f.DevPodProviderOptionsCheckNamespaceDescription( | ||
| ctx, | ||
| "provider2", | ||
| "The namespace to use", | ||
| ) | ||
| framework.ExpectNoError(err) | ||
| cancel() | ||
|
|
||
| // Update provider 2 (change the namespace description value) | ||
| err = f.DevPodProviderUpdate( | ||
| ctx, | ||
| "provider2", | ||
| tempDir+"/provider2-update.yaml", | ||
| ) | ||
| framework.ExpectNoError(err) | ||
|
|
||
| // Ensure that provider 2 was updated | ||
| ctx, cancel = context.WithDeadline( | ||
| ctx, | ||
| time.Now().Add(30*time.Second), | ||
| ) | ||
| err = f.DevPodProviderOptionsCheckNamespaceDescription( | ||
| ctx, | ||
| "provider2", | ||
| "Updated namespace parameter", | ||
| ) | ||
| framework.ExpectNoError(err) | ||
| cancel() | ||
|
|
||
| // Cleanup: delete provider 2 | ||
| err = f.DevPodProviderDelete(ctx, "provider2") | ||
| framework.ExpectNoError(err) |
There was a problem hiding this comment.
Context is used after cancellation, causing operations to fail.
The pattern here shadows ctx with a deadline-derived context and then calls cancel(). After cancel() on line 87, the subsequent operations on lines 90-95 use the cancelled context. Similarly, after cancel() on line 108, the cleanup operations on lines 111-116 use the cancelled context.
This will cause DevPodProviderUpdate and DevPodProviderDelete to fail immediately with context canceled.
🐛 Proposed fix: use separate variable names for derived contexts
// Ensure provider 2 namespace parameter has the default value
- ctx, cancel := context.WithDeadline(
+ checkCtx, cancel := context.WithDeadline(
ctx,
time.Now().Add(30*time.Second),
)
err = f.DevPodProviderOptionsCheckNamespaceDescription(
- ctx,
+ checkCtx,
"provider2",
"The namespace to use",
)
framework.ExpectNoError(err)
cancel()
// Update provider 2 (change the namespace description value)
err = f.DevPodProviderUpdate(
ctx,
"provider2",
tempDir+"/provider2-update.yaml",
)
framework.ExpectNoError(err)
// Ensure that provider 2 was updated
- ctx, cancel = context.WithDeadline(
+ checkCtx, cancel = context.WithDeadline(
ctx,
time.Now().Add(30*time.Second),
)
err = f.DevPodProviderOptionsCheckNamespaceDescription(
- ctx,
+ checkCtx,
"provider2",
"Updated namespace parameter",
)
framework.ExpectNoError(err)
cancel()📝 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.
Suggested change
| ctx, cancel := context.WithDeadline( | |
| ctx, | |
| time.Now().Add(30*time.Second), | |
| ) | |
| err = f.DevPodProviderOptionsCheckNamespaceDescription( | |
| ctx, | |
| "provider2", | |
| "The namespace to use", | |
| ) | |
| framework.ExpectNoError(err) | |
| cancel() | |
| // Update provider 2 (change the namespace description value) | |
| err = f.DevPodProviderUpdate( | |
| ctx, | |
| "provider2", | |
| tempDir+"/provider2-update.yaml", | |
| ) | |
| framework.ExpectNoError(err) | |
| // Ensure that provider 2 was updated | |
| ctx, cancel = context.WithDeadline( | |
| ctx, | |
| time.Now().Add(30*time.Second), | |
| ) | |
| err = f.DevPodProviderOptionsCheckNamespaceDescription( | |
| ctx, | |
| "provider2", | |
| "Updated namespace parameter", | |
| ) | |
| framework.ExpectNoError(err) | |
| cancel() | |
| // Cleanup: delete provider 2 | |
| err = f.DevPodProviderDelete(ctx, "provider2") | |
| framework.ExpectNoError(err) | |
| checkCtx, cancel := context.WithDeadline( | |
| ctx, | |
| time.Now().Add(30*time.Second), | |
| ) | |
| err = f.DevPodProviderOptionsCheckNamespaceDescription( | |
| checkCtx, | |
| "provider2", | |
| "The namespace to use", | |
| ) | |
| framework.ExpectNoError(err) | |
| cancel() | |
| // Update provider 2 (change the namespace description value) | |
| err = f.DevPodProviderUpdate( | |
| ctx, | |
| "provider2", | |
| tempDir+"/provider2-update.yaml", | |
| ) | |
| framework.ExpectNoError(err) | |
| // Ensure that provider 2 was updated | |
| checkCtx, cancel = context.WithDeadline( | |
| ctx, | |
| time.Now().Add(30*time.Second), | |
| ) | |
| err = f.DevPodProviderOptionsCheckNamespaceDescription( | |
| checkCtx, | |
| "provider2", | |
| "Updated namespace parameter", | |
| ) | |
| framework.ExpectNoError(err) | |
| cancel() | |
| // Cleanup: delete provider 2 | |
| err = f.DevPodProviderDelete(ctx, "provider2") | |
| framework.ExpectNoError(err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/provider/provider.go` around lines 77 - 112, The bug is that the
code shadows and cancels ctx then continues to reuse that cancelled context for
f.DevPodProviderUpdate and f.DevPodProviderDelete; fix by creating new derived
contexts (e.g., checkCtx, checkCancel := context.WithDeadline(ctx, ...)) for
each call to f.DevPodProviderOptionsCheckNamespaceDescription, call
checkCancel() after the check completes, and ensure f.DevPodProviderUpdate and
f.DevPodProviderDelete are invoked with the original (uncancelled) ctx or a new
dedicated context; update references to cancel/ctx accordingly so you never
reuse a cancelled context when calling DevPodProviderUpdate or
DevPodProviderDelete.
Signed-off-by: Samuel K <skevetter@pm.me>
This was referenced Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Samuel K skevetter@pm.me
Summary by CodeRabbit