Repository navigation
test: E2E tests for Go SDK - #3338
jiripetrlik wants to merge 1 commit into
Conversation
|
Hello @rhuss , I've tried to fix your comments related to assertion depth from #3028 (comment) and rebased PR. Can you please take a look again? |
rhuss
left a comment
rhuss
left a comment
There was a problem hiding this comment.
cc-review Summary
What Went Well
- The response-contract helpers are the real fix from the last round.
requireProviderResponse(providers_test.go:38-47) andrequireWorkspaceResponse(workspace_test.go:18-25) assert the full field set, including that credentials come back empty, which is exactly what the earlier review asked for. attachProviderRetryanddetachProviderRetry(providers_test.go:92-131) handle optimistic-concurrency conflicts by re-fetching ResourceVersion, rather than assuming the first write wins.- Test isolation is sound:
uniqueName(harness_test.go:128-130) plus consistentt.Cleanupregistration makest.Parallel()safe, and cleanup detaches providers before deleting sandboxes so no credential material is left attached. TestProviderCRUDResponseContract(providers_test.go:259-311) exercises Create, Get, Update and ListAll in sequence and checks ResourceVersion monotonicity at line 298.- A correctness pass over every SDK call against the signatures in
sdk/go/openshell/v1/found no misuse: no context lifetime errors, no resource leaks on failure paths, correct cleanup ordering under LIFOt.Cleanup. - The credential assertion tests a real security boundary correctly. Checked against
crates/openshell-core/src/secrets.rs, the suite genuinely fails if the gateway returns a raw secret, an empty value, or an encoded copy.
Findings
| Severity | File | Description | Source |
|---|---|---|---|
| Important | e2e/go/harness_test.go:104 | waitForPersistenceReady uses context.Background() with no deadline | production |
| Important | e2e/go/providers_test.go:238 | AttachProviderResult.Sandbox and DetachProviderResult.Sandbox never asserted | test-quality |
| Important | e2e/go/sandbox_lifecycle_test.go:27 | No sandbox error-path tests (NotFound, AlreadyExists) | test-quality, goal-alignment |
| Important | e2e/go/sandbox_lifecycle_test.go:30 | Sandbox Create under-asserted vs the workspace and provider helpers | test-quality |
| Important | e2e/go/sandbox_lifecycle_test.go:51 | No failing-command exec test | goal-alignment |
| Important | tasks/test.toml:64 | e2e:go puts a Podman requirement into the Docker-only e2e aggregate | production, architecture, goal-alignment |
| Important | tasks/test.toml:64 | PR claims GitHub Actions were updated, but no .github/ file is touched | goal-alignment, codex |
| Important | tasks/test.toml:140 | No -timeout on go test | production, goal-alignment |
| Minor | (10 findings) | helper placement, poll-helper duplication, stale comments, retry diagnostics, stderr and placeholder-matching details | various |
| Notable | (4 findings) | Spec round-trip and uncovered SDK surfaces, stable-handle placeholder form, RFC 0015 divergence | various |
Four of these were raised in the review of the predecessor implementation and are still open: the failing-exec test, the sandbox error-path tests, the Podman and Docker mismatch in the aggregate, and the missing -timeout. The one item that was fixed, discarded response values, was fixed thoroughly for providers and workspaces. The sandbox tests did not get the same treatment.
Not posted inline (outside the diff hunks):
tasks/test.toml:4and the[test]description on line 7 still read "Rust + Python + TypeScript SDK" and do not mention Go, although the[e2e]description on line 64 was updated.mise.lockremoves the sccache linux-x64 platform entry (5 lines). This is unrelated to the Go E2E work and undeclared. It looks like lock file churn from running mise on an ARM host; please confirm it was intentional.
Review Details
- Findings posted: 20 inline, 2 summary only
- Gate outcome: FAIL (8 Important, 0 Critical)
- Participating agents: correctness, architecture, security, production, test-quality, goal-alignment, codex, coderabbit
- Meta-review: 22 findings verified by a skeptic pass; 0 false positives removed, 6 marked weak, 1 severity raised. CodeRabbit reported no findings.
| func waitForPersistenceReady(client *v1.Client) error { | ||
| ctx := context.Background() | ||
| var lastErr error | ||
| for range 60 { | ||
| _, err := client.Sandboxes().ListAll(ctx, "default", v1.ListOptions{PageSize: 1}) | ||
| if err == nil { | ||
| return nil | ||
| } | ||
| lastErr = err | ||
| if v1.IsUnavailable(err) { | ||
| time.Sleep(2 * time.Second) | ||
| continue | ||
| } | ||
| if strings.Contains(err.Error(), "no such table: objects") { | ||
| time.Sleep(1 * time.Second) | ||
| continue | ||
| } | ||
| return fmt.Errorf("unexpected error waiting for persistence: %w", err) | ||
| } | ||
| return fmt.Errorf("openshell-server persistence is not initialized after 60 attempts: %w", lastErr) | ||
| } |
There was a problem hiding this comment.
Important: waitForPersistenceReady uses context.Background() with no deadline, so a hung gRPC call blocks TestMain indefinitely.
Why this matters: Line 105 creates ctx := context.Background(). The 60-iteration attempt cap on line 107 only guards against repeated short failures. If a single ListAll call hangs (TCP socket open but gateway process stuck), the loop blocks forever because the context has no deadline. This runs in TestMain before any test executes, so it blocks the whole binary with no diagnostics.
Suggested fix: ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute); defer cancel()
Source: production agent
There was a problem hiding this comment.
Added a three-minute deadline to gateway persistence readiness checks.
There was a problem hiding this comment.
Confirmed. harness_test.go:108 now wraps the readiness loop in context.WithTimeout(context.Background(), 3*time.Minute), so a hung ListAll can no longer block TestMain.
| attachResult := attachProviderRetry(ctx, t, client, "default", sandboxName, providerName) | ||
| require.True(t, attachResult.Attached) | ||
|
|
||
| value = pollUntil(t, ctx, func() string { | ||
| v, err := readEnvVar(ctx, client, "default", sandboxName, "NVIDIA_API_KEY") | ||
| require.NoError(t, err) | ||
| return v | ||
| }, func(v string) bool { return v != "NOT_SET" }) | ||
| require.True(t, isPlaceholderForEnvKey(value, "NVIDIA_API_KEY"), "expected a placeholder, got %q", value) | ||
|
|
||
| detachResult := detachProviderRetry(ctx, t, client, "default", sandboxName, providerName) | ||
| require.True(t, detachResult.Detached) |
There was a problem hiding this comment.
Important: AttachProviderResult.Sandbox and DetachProviderResult.Sandbox are never asserted. Both tests check only the boolean flag (Attached, Detached).
Why this matters: A gateway returning Attached: true with a nil or zero-valued Sandbox would pass. The embedded Sandbox is the primary mechanism for clients to observe post-mutation state (updated provider list, new resource version), so serialization bugs in the attach and detach response path cannot be detected.
Suggested fix: After attachProviderRetry assert NotNil(attachResult.Sandbox), Equal(sandbox.ID, attachResult.Sandbox.ID) and that the provider list contains the attached provider. Make the symmetric assertions after detach.
Source: test-quality agent
There was a problem hiding this comment.
Added assertions for the returned sandbox identity and provider attachment state.
There was a problem hiding this comment.
Confirmed, and you went further than the comment asked. Both results now run through requireSandboxResponse, assert the sandbox identity against the created ID, and check Spec.Providers membership with Contains after attach and NotContains after detach. That covers the post-mutation state the boolean flag was hiding.
| sb, err := client.Sandboxes().Create(ctx, "default", name, &v1.SandboxSpec{ | ||
| Policy: defaultPolicy(), | ||
| }, nil) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 2*time.Minute) | ||
| defer cleanupCancel() | ||
| require.NoError(t, client.Sandboxes().Delete(cleanupCtx, "default", name)) | ||
| }) | ||
| require.NotEmpty(t, sb.ID) | ||
| require.Equal(t, name, sb.Name) | ||
|
|
||
| ready, err := client.Sandboxes().WaitReady(ctx, "default", name) | ||
| require.NoError(t, err) | ||
| requireReadySandbox(t, ready, sb.ID) | ||
|
|
||
| fetched, err := client.Sandboxes().Get(ctx, "default", name) | ||
| require.NoError(t, err) | ||
| require.Equal(t, sb.ID, fetched.ID) | ||
|
|
||
| sandboxes, err := client.Sandboxes().ListAll(ctx, "default", v1.ListOptions{PageSize: 100}) | ||
| require.NoError(t, err) | ||
| require.True(t, containsID(sandboxes, sb.ID)) | ||
|
|
||
| result, err := client.Exec().Run(ctx, "default", name, []string{"sh", "-c", "printf sandbox-ok"}, v1.ExecOptions{}) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 0, result.ExitCode) | ||
| require.Equal(t, "sandbox-ok", string(result.Stdout)) | ||
|
|
||
| // Exec launches share the same sandbox filesystem across calls. | ||
| writeResult, err := client.Exec().Run(ctx, "default", name, | ||
| []string{"sh", "-c", "echo persisted > /sandbox/exec-persistence.txt"}, v1.ExecOptions{}) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 0, writeResult.ExitCode) | ||
|
|
||
| readResult, err := client.Exec().Run(ctx, "default", name, | ||
| []string{"cat", "/sandbox/exec-persistence.txt"}, v1.ExecOptions{}) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 0, readResult.ExitCode) | ||
| require.Equal(t, "persisted\n", string(readResult.Stdout)) |
There was a problem hiding this comment.
Important: No error-path tests for sandbox operations: Get on a nonexistent sandbox (NotFound), Create with a duplicate name (AlreadyExists), Delete on a nonexistent sandbox. The workspace tests cover both NotFound cases.
Why this matters: The SDK error helpers IsNotFound and IsAlreadyExists depend on correct gRPC status mapping through the converter layer. If the gateway returns a different status for a sandbox 404 than for a workspace 404, the current suite would not catch it. Flagged as R3 in the prior review and unresolved.
Suggested fix: Add TestSandboxGetNonexistentNotFound and TestSandboxDeleteNonexistentNotFound mirroring the workspace tests; optionally TestSandboxCreateDuplicateAlreadyExists.
Source: test-quality agent (also flagged by: goal-alignment)
There was a problem hiding this comment.
Added NotFound coverage for get/delete and AlreadyExists coverage for duplicate creation.
There was a problem hiding this comment.
Confirmed. TestSandboxGetNonexistentNotFound, TestSandboxDeleteNonexistentNotFound and TestSandboxCreateDuplicateAlreadyExists now give the sandbox path the same error-classification coverage the workspace tests had.
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 2*time.Minute) | ||
| defer cleanupCancel() | ||
| require.NoError(t, client.Sandboxes().Delete(cleanupCtx, "default", name)) | ||
| }) | ||
| require.NotEmpty(t, sb.ID) | ||
| require.Equal(t, name, sb.Name) |
There was a problem hiding this comment.
Important: Sandbox Create response is under-asserted relative to the workspace and provider equivalents. Only ID (non-empty) and Name are checked; Workspace, CreatedAt and ResourceVersion are not.
Why this matters: A gateway returning an empty Workspace, zero CreatedAt or zero ResourceVersion on sandbox Create would pass every test. For a suite whose stated purpose is catching protocol schema mismatches, leaving the primary resource type less validated than the secondary ones is a real gap. requireProviderResponse and requireWorkspaceResponse prove the pattern already exists in this same suite.
Suggested fix: Add requireSandboxResponse(t, sandbox, workspace, name) asserting NotNil, NotEmpty(ID), Equal(workspace, Workspace), Equal(name, Name), False(CreatedAt.IsZero()), NotZero(ResourceVersion). Use it after Create and Get in TestSandboxCRUDAndExec and after each Create in TestSandboxListScopedAndAllWorkspaces.
Source: test-quality agent
There was a problem hiding this comment.
Added shared assertions for ID, workspace, name, creation time, and resource version.
There was a problem hiding this comment.
Confirmed. requireSandboxResponse in harness_test.go:137 asserts ID, workspace, name, CreatedAt and ResourceVersion, and it is applied after both Create and Get. The sandbox response contract now matches the provider and workspace helpers.
| result, err := client.Exec().Run(ctx, "default", name, []string{"sh", "-c", "printf sandbox-ok"}, v1.ExecOptions{}) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 0, result.ExitCode) | ||
| require.Equal(t, "sandbox-ok", string(result.Stdout)) |
There was a problem hiding this comment.
Important: Exec is tested only on the happy path. No test runs a failing command and asserts a non-zero ExitCode or stderr content.
Why this matters: Without a failing-command test the suite cannot detect regressions where the gateway swallows non-zero exit codes or drops stderr. Flagged as R2 in the prior review and unresolved.
Suggested fix: Add a case running sh -c 'echo err >&2; exit 42' asserting ExitCode == 42 and that stderr contains the sentinel.
Source: goal-alignment agent
There was a problem hiding this comment.
Added a nonzero-exit execution test. I still need to require exact stderr and enable NoLoginShell for RFC-0015 compliance.
There was a problem hiding this comment.
Confirmed for the exit code and stdout: the new case asserts exit 42, empty stdout and the stderr sentinel. Agreed on your note that exact stderr equality and NoLoginShell should wait for the RFC 0015 exec definition, which now pins tty=false and no_login_shell=true. Worth doing in the same change that aligns the adapter.
| result, err := client.Exec().Run(ctx, "default", name, []string{"sh", "-c", "printf sandbox-ok"}, v1.ExecOptions{}) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 0, result.ExitCode) | ||
| require.Equal(t, "sandbox-ok", string(result.Stdout)) |
There was a problem hiding this comment.
Minor: Exec success results never assert that Stderr is empty. All exec calls check ExitCode and Stdout but discard Stderr (lines 51-54, 58-60, 62-66).
Why this matters: A gateway that erroneously includes error output in a successful execution would pass undetected. Note that this assertion is only well defined for non-TTY exec, since a PTY merges the two streams.
Suggested fix: Add require.Empty(t, result.Stderr) after each successful exec assertion, and confirm the exec runs without a TTY.
Source: test-quality agent
There was a problem hiding this comment.
Added empty-stderr assertions to successful execution paths.
There was a problem hiding this comment.
Confirmed. require.Empty on Stderr is present on all three successful exec paths.
| func elementsMatch(a, b []string) bool { | ||
| if len(a) != len(b) { | ||
| return false | ||
| } | ||
| counts := make(map[string]int, len(a)) | ||
| for _, v := range a { | ||
| counts[v]++ | ||
| } | ||
| for _, v := range b { | ||
| counts[v]-- | ||
| } | ||
| for _, c := range counts { | ||
| if c != 0 { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Minor: elementsMatch hand-rolls unordered set equality by map counting.
Why this matters: Go 1.25 provides slices.Sorted and slices.Equal in the standard library, which would replace the counting logic with a one-liner and remove the surface for off-by-one bugs. It exists because the poll helper needs a boolean predicate rather than testify's fatal require.ElementsMatch.
Suggested fix: Use slices.Equal(slices.Sorted(slices.Values(a)), slices.Sorted(slices.Values(b))), or fold it into the generic pollUntil[T].
Source: architecture agent
There was a problem hiding this comment.
Replaced the counting implementation with sorted slice comparison.
There was a problem hiding this comment.
Confirmed. slices.Equal(slices.Sorted(slices.Values(a)), slices.Sorted(slices.Values(b))) replaces the map counting.
| func uniqueName(prefix string) string { | ||
| return fmt.Sprintf("%s-%09d", prefix, time.Now().UnixNano()%1_000_000_000) | ||
| } |
There was a problem hiding this comment.
Notable: Draft RFC 0015 (PR #3238) proposes shared scenario identifiers across SDK E2E suites and a task name of the form e2e:sdk:go. The current task is e2e:go and the tests carry no scenario identifiers.
Why this matters: The RFC is not normative yet so no action is required. The file-per-domain structure (sandbox, workspace, providers) aligns with the RFC's proposed groupings, which would ease a later migration.
Suggested fix: No fix now. If RFC 0015 is adopted, rename the task and annotate each test with its scenario identifier.
Source: architecture agent
There was a problem hiding this comment.
Agreed. The current task split predates the RFC contract; it still needs e2e:sdk:go:adapter, Docker-backed e2e:sdk:go, and the e2e:go alias.
There was a problem hiding this comment.
Acknowledged, and your read matches mine. Nothing to do here until RFC 0015 is accepted; the task split and scenario identifiers belong in the same change that introduces the adapter contract.
| func isPlaceholderForEnvKey(value, key string) bool { | ||
| const prefix = "openshell:resolve:env:" | ||
| if value == prefix+key { | ||
| return true | ||
| } | ||
| token, ok := strings.CutPrefix(value, prefix) | ||
| if !ok { | ||
| return false | ||
| } | ||
| return strings.HasPrefix(token, "v") && strings.HasSuffix(token, "_"+key) | ||
| } |
There was a problem hiding this comment.
Notable: isPlaceholderForEnvKey does not recognize the stable-handle placeholder variant openshell:resolve:env:s<64 lowercase hex>_KEY defined in crates/openshell-core/src/secrets.rs (split_stable_env_key, lines 802-815).
Why this matters: The security invariant is preserved: a stable-handle placeholder makes the test fail rather than silently pass, so a leaked secret is still caught. But if the gateway begins issuing stable handles for refresh-managed credentials, these tests produce false failures with a misleading diagnostic that does not indicate the value is a valid placeholder in another format.
Suggested fix: Extend isPlaceholderForEnvKey to accept the stable-handle form, ideally with one compiled regexp covering all three variants: ^openshell:resolve:env:(?:KEY|v[0-9]+_KEY|s[0-9a-f]{64}_KEY)$
Source: security agent
There was a problem hiding this comment.
Added strict support for the s<64-lowercase-hex>_ placeholder form.
There was a problem hiding this comment.
Confirmed. The s<64 lowercase hex>_KEY form is now accepted with a strict length and hex check, which matches split_stable_env_key in crates/openshell-core/src/secrets.rs. The separate TestIsPlaceholderForEnvKey unit test is a good addition, since the matcher is now the security assertion's load-bearing piece.
| sb, err := client.Sandboxes().Create(ctx, "default", name, &v1.SandboxSpec{ | ||
| Policy: defaultPolicy(), | ||
| }, nil) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 2*time.Minute) | ||
| defer cleanupCancel() | ||
| require.NoError(t, client.Sandboxes().Delete(cleanupCtx, "default", name)) | ||
| }) | ||
| require.NotEmpty(t, sb.ID) | ||
| require.Equal(t, name, sb.Name) |
There was a problem hiding this comment.
Notable: Sandbox Spec round-trip is never verified, and several SDK surfaces are uncovered: Stop, Start, WaitStopped, Watch, GetLogs, ListProviders, Exec Stream and Interactive, and the paginated List path.
Why this matters: Sandboxes are created with defaultPolicy() containing filesystem, landlock and process policy, but the returned or fetched Spec.Policy is never compared. Policy serialization across the gRPC boundary is exactly the kind of schema mismatch this suite exists to catch. These are scope decisions rather than bugs and are reasonable for a first E2E PR.
Suggested fix: Consider a TestSandboxSpecRoundTrip that creates with a detailed policy, fetches via Get and deep-compares the Spec. Track the remaining interface methods and paginated List as follow-up coverage.
Source: test-quality agent
There was a problem hiding this comment.
Added policy round-trip assertions and stronger CRUD coverage. Scenario IDs, coverage reporting, scavenging, and remaining RFC error cases are still outstanding.
There was a problem hiding this comment.
Confirmed for the round trip: the policy is compared against Spec.Policy after both Create and Get, which is the schema-mismatch case I cared about most. The remaining surfaces (Stop, Start, Watch, GetLogs, paginated List, Exec streaming) stay as follow-up coverage, and scenario IDs, coverage reporting and scavenging belong with the RFC 0015 alignment.
f938393 to
8313274
Compare
|
Hello @rhuss , thank you for your review. I've tried to fix all the issues. See comments for individual problems. Here is also how I've fixed additional comments:
|
|
Second pass on Three of the fixes are worth calling out because they went past what was asked. The Podman and Docker split landed in the right shape: Beyond the findings, the attach and detach assertions now check The three items you deferred are the right ones to defer, and I agree with your reasoning on each: exact stderr with Nothing blocking remains from my side. I cannot resolve review threads on this repository, so the 20 threads need resolving on your end. After that this needs a vetter to comment |
|
@mrunalp @sjenning @krishicks I think we are good to go with this PR (along with the associated RFC), to at least run it through the CI pipeline. It really would help to keep the Golang SDK stable over the long run and detect regressions earlier. (i'm off next week, therefor this eager ping :-) |
Signed-off-by: Jiri Petrlik <jpetrlik@redhat.com>
8313274 to
00cd9ae
Compare
|
Hello @mrunalp , @sjenning , @krishicks , can you please take a look at this PR? I've rebased it with current main. It will help us to better test Go SDK using E2E tests. |
|
would be really great if we could integrate the tests, either now or when we moved to the CNFC repos (under another GitHub org then, I guess). @jiripetrlik wdyt, would it be better to wait to this move (should happen quite soon, now that OpenShell has been accepted for the CNCF sandbox) or does it not really matter? |
|
@rhuss I would prefer to merge soon. It is open for quite a long time and I don't want it to deteriorate. |
|
Label |
|
/ok to test 00cd9ae |
|
Thank you @sjenning for running the PR checks. Everything looks green, so I think this is ready for review. Could you please take a look? |
|
This pull request has had no activity for 14 days and is now marked stale. |
github-actions
Bot
added
the
state:stale
|
@jiripetrlik Sorry for the long delay on this one. Things should get better once the Go SDK has its own repository, and I'm starting that migration this week. I re-checked my 20 review threads against the rebased head |
|
I would have another round of reviews with an improved team of agents, so I post it here. Not sure if we should wait until the migration, but you might point your agent on this to check. |
rhuss
left a comment
rhuss
left a comment
There was a problem hiding this comment.
cc-review Summary
New round with fresh agent eyes on 00cd9ae, the squashed head since my September round. The earlier threads are addressed. The coverage gaps still open from that round (stop/start, exec options and streaming) and the planned removal of the conflict retries per RFC 0015 stay in their existing threads, so I don't repeat them here. These findings come from an agent-assisted review; the failure scenarios were derived from the code, not executed.
What Went Well
- The write-only credential contract is pinned from both sides:
requireProviderResponseasserts an emptySpec.Credentialson every Create/Get/Update/List response, andTestAttachDetachProviderUpdatesCredentialschecks from inside the sandbox that the variable isNOT_SETbefore attach, a placeholder (never the raw secret) after attach, andNOT_SETagain after detach (e2e/go/providers_test.go). isPlaceholderForEnvKeyparses strictly (digit-only revision, 64 lowercase hex stable handle, exact key match), and the table-drivenTestIsPlaceholderForEnvKeycovers the overlapping-suffix casev12_LONG_API_KEYvsAPI_KEY(e2e/go/harness_test.go).readEnvVarretries only the relay-openDeadlineExceeded, once and only while the parent ctx is live, and passes the env key as a positional argument tosh -cinstead of interpolating it into the script (e2e/go/harness_test.go).TestSandboxListScopedAndAllWorkspaceschecks workspace scoping in both directions plus inclusion underAllWorkspaces, andTestSandboxCRUDAndExecchecks the non-zero exit path precisely (exit code 42, empty stdout, stderr marker) (e2e/go/sandbox_lifecycle_test.go).TestMainreusesgateway.LoadConfig/gateway.NewClientandmtlsClientOptionadds the on-disk mTLS bundle only when present, so the suite takes the same SDK connection path as users across plaintext, OIDC and mTLS lanes (e2e/go/harness_test.go).- The separate
e2e/go/go.modwith areplaceonto../../sdk/gotests the in-tree SDK while keeping testify and other e2e-only dependencies out of the SDK module.
Findings
| Severity | File | Description | Source |
|---|---|---|---|
| Minor | e2e/go/harness_test.go:33 |
Missing OPENSHELL_GATEWAY turns into skips, so the e2e lane can go green with no coverage |
gator |
| Minor | e2e/go/harness_test.go:131 |
uniqueName comment claims process-unique names; wall-clock nanos can collide |
architecture |
| Notable | e2e/go/harness_test.go:217 |
TestIsPlaceholderForEnvKey is a unit test behind the e2e tag, so only gateway lanes run it |
architecture |
| Minor | e2e/go/harness_test.go:245 |
No per-attempt timeout in pollUntil/waitForPersistenceReady; one hung RPC eats 3 minutes |
production |
| Minor | e2e/go/providers_test.go:51 |
detachProviderBestEffort drops a Conflict, so the provider delete in cleanup can fail |
correctness |
| Minor | e2e/go/providers_test.go:289 |
Provider update with nil Credentials never checks the stored credential survived |
test-quality |
| Minor | e2e/go/sandbox_lifecycle_test.go:117 |
require assertions run before t.Cleanup is registered, leaking resources on failure |
correctness |
| Minor | e2e/go/sandbox_lifecycle_test.go:125 |
Duplicate-create test does not check the existing sandbox stayed unchanged | test-quality |
| Minor | e2e/go/sandbox_lifecycle_test.go:148 |
Cleanups discard delete errors and the workspace drain gives up silently after 20s | test-quality |
| Minor | e2e/go/sandbox_lifecycle_test.go:155 |
Sandbox cleanup block copied into ~8 closures with different error policies | architecture |
| Minor | e2e/go/sandbox_lifecycle_test.go:158 |
Sandbox Delete errors in cleanup discarded with _, _ =, leaking sandboxes silently |
correctness |
| Notable | e2e/go/workspace_test.go:27 |
Workspace tests skip the membership API (AddMember, RemoveMember, ListMembers) |
goal-alignment |
| Minor | tasks/test.toml:164 |
e2e/go module is outside the Go lint and format gates (go:lint, go:format:check) |
architecture |
All findings are posted as inline comments. Three of them (on sandbox_lifecycle_test.go lines 148, 155 and 158) look at the same cleanup error handling from different angles, so one helper with a single error policy would address all three.
Review Details
- Findings posted: 13
- Findings reviewed and not posted: 6
- Gate outcome: PASS (no Critical or Important findings)
- Participating reviewers: architecture, correctness, gator, goal-alignment, production, test-quality
| ) | ||
|
|
||
| func TestMain(m *testing.M) { | ||
| gatewayName = os.Getenv("OPENSHELL_GATEWAY") |
There was a problem hiding this comment.
Minor: TestMain turns a missing OPENSHELL_GATEWAY into per-test skips. The e2e:go and e2e:go:docker lanes run go test without -v, so if the wrapper ever stops exporting the variable, the lane still reports a green ok with no gateway coverage at all.
Why this matters: An e2e lane should fail when it cannot reach the system under test. With the skip in requireClient (lines 95-101), the job prints only ok github.com/NVIDIA/OpenShell/e2e/go whether 16 tests ran or all of them were skipped; only TestIsPlaceholderForEnvKey always runs. This is not reachable today, since both wrappers export the variable (with-docker-gateway.sh:708, with-podman-gateway.sh:645). A later change to either wrapper, or a manual go test -tags=e2e ./..., would silently drop the Go SDK coverage from a required CI job.
Suggested fix: The file is already gated by the e2e build tag, so fail fast instead of skipping. In TestMain:
if gatewayName == "" {
fmt.Fprintln(os.Stderr, "e2e: OPENSHELL_GATEWAY must be set (run via mise run e2e:go)")
os.Exit(1)
}and drop the t.Skip branch in requireClient. Adding -v to the go test call would at least make the skips visible in CI logs. Reproducer: cd e2e/go && env -u OPENSHELL_GATEWAY go test -tags=e2e ./... should print ok and exit 0 although no gateway test ran.
Source: gator agent
| return fmt.Errorf("openshell-server persistence is not initialized after 60 attempts: %w", lastErr) | ||
| } | ||
|
|
||
| // uniqueName returns a process-unique resource name so parallel tests never |
There was a problem hiding this comment.
Minor: The doc comment says uniqueName returns a "process-unique" name, but the value is wall-clock UnixNano modulo 1e9. That is neither monotonic nor guaranteed unique, so the comment promises more than the code delivers.
Why this matters: Two calls with the same prefix in the same clock tick return the same name. macOS reports time at microsecond resolution, so UnixNano values move in steps of 1000. Parallel subtests or a future helper that calls uniqueName("sb-crud") twice could collide, although the comment says that cannot happen.
Suggested fix: Append a package-level atomic counter, for example fmt.Sprintf("%s-%d-%d", prefix, time.Now().UnixNano()%1e9, seq.Add(1)), so the comment becomes true. Otherwise reword the comment to say the name is probabilistically unique.
Source: architecture agent
| } | ||
| } | ||
|
|
||
| func TestIsPlaceholderForEnvKey(t *testing.T) { |
There was a problem hiding this comment.
Notable: TestIsPlaceholderForEnvKey is a pure unit test of a parsing helper, but it lives in the harness file behind the e2e build tag. It only runs through the gateway-booting e2e:go tasks and never in the fast unit-test path.
Why this matters: If a change to the placeholder grammar (the v<digits> and s<64 hex> discriminators) breaks isPlaceholderForEnvKey, the table test that would catch it runs only after a full Docker or Podman gateway has booted. go:test and go:ci never run it.
Suggested fix: Consider moving the parser and its test into a file without the build tag, or into sdk/go if the SDK should own placeholder recognition.
Source: architecture agent
|
|
||
| // pollUntil calls fetch until the predicate is satisfied, the local polling | ||
| // timeout expires, or ctx is done, whichever happens first. | ||
| func pollUntil[T any](t *testing.T, ctx context.Context, fetch func() T, predicate func(T) bool) T { |
There was a problem hiding this comment.
Minor: The 35s deadline in pollUntil and the attempt loop in waitForPersistenceReady only take effect between calls. Each fetch or ListAll call runs on the caller's long-lived context with no per-attempt timeout, so a single hung RPC blocks for the full outer context (3 minutes) and is then misreported.
Why this matters: In providers_test.go:235, pollUntil calls readEnvVar(ctx, ...) with the test's 3-minute ctx. If the exec relay hangs (for example during a supervisor reconnect while attaching), Exec().Run blocks until that ctx expires, and the 35s timer at harness_test.go:247 is never selected. The require.NoError inside the fetch closure then fails with DeadlineExceeded, which hides where the hang happened. waitForPersistenceReady (harness_test.go:112-127) has the same shape: a ListAll call against a gateway that accepts connections but never answers uses the whole budget, and because DeadlineExceeded is not IsUnavailable, it reports "unexpected error waiting for persistence" instead of a readiness timeout.
Suggested fix: Derive a short per-attempt context inside each fetch or ListAll attempt, for example context.WithTimeout(ctx, 10*time.Second), and check ctx.Err() before classifying the error.
Source: production agent
| // synchronously first. | ||
| func detachProviderBestEffort(ctx context.Context, client *v1.Client, workspace, sandboxName, providerName string) { | ||
| if current, err := client.Sandboxes().Get(ctx, workspace, sandboxName); err == nil { | ||
| _, _ = client.Sandboxes().DetachProvider(ctx, workspace, sandboxName, providerName, current.ResourceVersion) |
There was a problem hiding this comment.
Minor: detachProviderBestEffort makes a single detach attempt and discards its error. An optimistic-concurrency conflict therefore leaves the provider attached, and the later provider delete in cleanup fails with FailedPrecondition.
Why this matters: In the TestProviderCredentialsAvailableAsEnvVar cleanup, Get returns resource version N, a sandbox status update bumps it to N+1 before DetachProvider runs, and the resulting Conflict is dropped by _, _ =. Sandbox Delete removes the store row asynchronously, so when deleteProviderIgnoreNotFound calls DeleteProvider, delete_provider_record_with_credentials (crates/openshell-server/src/grpc/provider.rs:595-600) still finds the sandbox in sandboxes_using_provider and returns failed_precondition. That is not NotFound, so require.NoError fails the test during cleanup.
Suggested fix: The file already handles this race with attachProviderRetry/detachProviderRetry (50 attempts on IsConflict). Reuse a conflict-retrying detach in the cleanup path, or retry the provider delete on FailedPrecondition until the sandbox row is gone.
Source: correctness agent
| // sandbox still references it, so wait for the workspace to drain | ||
| // before attempting the delete. | ||
| waitForWorkspaceSandboxesGone(cleanupCtx, client, otherWorkspace) | ||
| _, _ = client.Workspaces().Delete(cleanupCtx, otherWorkspace) |
There was a problem hiding this comment.
Minor: Several cleanups throw away delete errors with _, _ =, and the workspace drain gives up silently after 20s, so sandboxes and workspaces can leak on the shared gateway without anyone noticing.
Why this matters: TestSandboxListScopedAndAllWorkspaces (lines 148, 158, 168) and TestSandboxLabelsAndSelectors (lines 211, 225) ignore Delete errors. waitForWorkspaceSandboxesGone returns quietly when its 20s deadline passes, and the workspace Delete after it is unchecked as well. Other tests in this file (lines 37, 121) and in workspace_test.go use require.NoError in cleanup. If sandbox deletion is slow or fails under -parallel 5, the extra workspace and its sandboxes stay on the gateway, the run reports green, and the leftovers pile up in later runs against a reused gateway.
Suggested fix: In cleanup, report anything other than IsNotFound with t.Errorf, and have waitForWorkspaceSandboxesGone log through t.Logf (or t.Errorf) when the deadline passes.
Source: test-quality agent
| defaultSandbox, err := client.Sandboxes().Create(ctx, "default", defaultName, &v1.SandboxSpec{}, nil) | ||
| require.NoError(t, err) | ||
| requireSandboxResponse(t, defaultSandbox, "default", defaultName) | ||
| t.Cleanup(func() { |
There was a problem hiding this comment.
Minor: The sandbox cleanup block (a fresh context.WithTimeout, defer cancel, then Sandboxes().Delete) is copied into roughly eight t.Cleanup closures, and the copies follow different error policies. Some assert with require.NoError (lines 34-39, 118-123, providers_test.go:146-152), others discard the error with _, _ = (lines 155-159, 165-169, 205-226).
Why this matters: When a sandbox delete fails during cleanup, for example with a transient Unavailable error, TestSandboxCRUDAndExec and TestSandboxCreateDuplicateAlreadyExists fail, while TestSandboxListScopedAndAllWorkspaces and TestSandboxLabelsAndSelectors leak the sandbox without any signal. A reader cannot tell whether the difference is intentional.
Suggested fix: Add one cleanupSandbox(t, client, workspace, name) helper in harness_test.go, mirroring deleteProviderIgnoreNotFound, that owns the cleanup timeout and a single error policy (for example, ignore NotFound and fail on everything else), and use it everywhere.
Source: architecture agent
| t.Cleanup(func() { | ||
| cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| defer cleanupCancel() | ||
| _, _ = client.Sandboxes().Delete(cleanupCtx, "default", defaultName) |
There was a problem hiding this comment.
Minor: Sandbox Delete errors in cleanup are discarded with _, _ = and nothing explains why, so a failed cleanup leaks sandboxes silently. Other tests in the same file assert NoError on the same call.
Why this matters: In TestSandboxListScopedAndAllWorkspaces (lines 158, 168) and TestSandboxLabelsAndSelectors (lines 211, 225), cleanup calls client.Sandboxes().Delete and drops the result. If the delete fails, for example with Unavailable or a cleanupCtx deadline, the test still passes and the sandbox stays on the gateway. In the workspace test, the workspace delete at line 148 then fails too, and that error is discarded as well. TestSandboxCRUDAndExec (line 37) and TestSandboxCreateDuplicateAlreadyExists (line 121) call require.NoError on the same cleanup.
Suggested fix: Ignore only NotFound, as deleteProviderIgnoreNotFound does, or pass DeleteOptions{AllowMissing: true}, and assert every other error.
Source: correctness agent
| require.NotZero(t, workspace.ResourceVersion) | ||
| } | ||
|
|
||
| func TestWorkspaceCRUD(t *testing.T) { |
There was a problem hiding this comment.
Notable: The workspace management tests cover Create, Get, ListAll and Delete, but not the SDK's membership API (AddMember, RemoveMember, ListMembers in sdk/go/openshell/v1/workspace_client.go:98-168), and the PR does not say whether this is deliberate.
Why this matters: The PR says the suite catches protocol mismatches for workspace management, yet a schema change to the member RPCs (role enum, member identity fields) would go unnoticed.
Suggested fix: Membership may need an OIDC lane, which the Docker e2e lane does not run. If so, limit the goal in the PR description to workspace CRUD, or add a member test to an OIDC-enabled lane.
Source: goal-alignment agent
| "OPENSHELL_E2E_DOCKER_SANDBOX_IMAGE=openshell/e2e-python:dev OPENSHELL_E2E_DOCKER_SANDBOX_IMAGE_PULL_POLICY=never e2e/with-docker-gateway.sh uv run pytest -o python_files='test_*.py *_test.py' -m 'not gpu' -n ${E2E_PARALLEL:-5} e2e/python", | ||
| ] | ||
|
|
||
| ["e2e:go"] |
There was a problem hiding this comment.
Minor: The new e2e/go module sits outside every Go lint and format gate. go:lint, go:format:check and go:build all run with dir = "sdk/go", so golangci-lint and gofmt never check the e2e module. The only static check is the go vet inside the gateway-wrapped e2e:go and e2e:go:docker commands.
Why this matters: An unformatted or lint-violating file under e2e/go/ (an unused helper, a shadowed err) still passes mise run go:ci in branch-checks.yml, because tasks/go.toml scopes those tasks to sdk/go and e2e/go/go.mod is a separate module. golangci-lint would also skip these files without --build-tags=e2e, since every file starts with //go:build e2e. The problem only shows up as a go vet failure after with-docker-gateway.sh has already booted a gateway.
Suggested fix: Add go:e2e:lint and go:e2e:format:check tasks with dir = "e2e/go" that run golangci-lint run --build-tags=e2e against sdk/go/.golangci.yml plus gofmt -l ., include them in go:ci, and remove go vet from the gateway-wrapped e2e commands.
Source: architecture agent
Summary
This PR adds E2E tests for Go SDK. New E2E Go SDK test suite should be able to catch issues with:
This PR should replace: jiripetrlik#1
Related Issue
Changes
Testing
mise run pre-commitpassesChecklist