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
github-actions
Bot
removed
the
state:stale
|
Heads-up: the Go SDK moves to ai-openshell/sdk-go on 2026-11-03 (details in #5031). The Go E2E suite is wanted in the new repository, where it runs against the published gateway image of every core release. So |
The suite from NVIDIA/OpenShell#3338 predates the switch of Sandboxes().Create from a labels map to CreateOption values. Pass labels through v1.WithLabels and drop the nil maps. Signed-off-by: Roland Huß <rhuss@redhat.com>
The suite from NVIDIA/OpenShell#3338 predates the switch of Sandboxes().Create from a labels map to CreateOption values. Pass labels through v1.WithLabels and drop the nil maps. Signed-off-by: Roland Huß <rhuss@redhat.com>
The suite from NVIDIA/OpenShell#3338 predates the switch of Sandboxes().Create from a labels map to CreateOption values. Pass labels through v1.WithLabels and drop the nil maps. Signed-off-by: Roland Huß <rhuss@redhat.com>
…lockstep releases (#6) * build: list the vendored conformance fixtures in one file conformance:check reads hack/conformance-fixtures.txt, so the core bump automation can refresh the same fixtures it checks. Signed-off-by: Roland Huß <rhuss@redhat.com> * feat(hack): list new core tags and open update proposals core-tags.sh prints core release tags newer than the pin: every newer stable tag and the newest pre-release if it is ahead of them (--all for every tag). open-bump-pr.sh builds branch core/<tag> in a temporary worktree, moves the pin, regenerates the bindings, refreshes the vendored fixtures, and opens a PR labeled core-bump plus stable or prerelease with the proto diff between the two core tags. It skips tags that already have a branch or a merged proposal, closes older pre-release proposals, and closes proposals whose core tag was deleted. --dry-run changes nothing on GitHub. Signed-off-by: Roland Huß <rhuss@redhat.com> * feat(hack): tag the lockstep SDK release for a stable pin tag-release.sh creates the SDK tag vX.Y.Z when a commit on main moves the pin to stable core vX.Y.Z, publishes the GitHub release with the merged proposal's body, and asks the Go module proxy for the version. Pre-release pins and commits that leave the pin alone create nothing, and an existing tag at another commit fails the run. Signed-off-by: Roland Huß <rhuss@redhat.com> * ci: watch core releases and tag SDK releases core-watch runs hourly and on dispatch, and opens a proposal per new core tag. release-tag runs on pushes to main and on dispatch for a given commit. Both use the ai-openshell-sdk-bot App token, scoped to the permissions they need, so their pushes and PRs trigger CI. Failures are reported through report-failure.sh, a job-summary stub until the drift issue exists. Adds the core:tags, core:bump, and release:tag tasks. Signed-off-by: Roland Huß <rhuss@redhat.com> * feat(hack): check core main for drift against the pin drift-check.sh runs buf breaking (FILE rules) from core main against the pinned tag for the proto files the SDK generates, regenerates the bindings from main in a temporary worktree and builds and vets against them, and lists RPCs the SDK does not call yet. It writes a markdown report with a 'Pin | Core main | Result' header and exits 1 on breaking changes or a failed build. Uncovered RPCs are informational. Signed-off-by: Roland Huß <rhuss@redhat.com> * feat(hack): keep one drift issue and record automation failures update-drift-issue.sh writes the drift report into the single open 'Core main drift' issue (label core-drift), creating it on drift and closing it once the check passes and no failures are pending. report-failure.sh appends a dated line to the issue's 'Automation failures' section, which the report updates carry over. Both take --dry-run. Signed-off-by: Roland Huß <rhuss@redhat.com> * ci: run the core drift check nightly core-drift runs drift-check.sh and update-drift-issue.sh with an App token scoped to issues. A run that produces no report is recorded as an automation failure. Signed-off-by: Roland Huß <rhuss@redhat.com> * ci: shellcheck the hack scripts workflows:check also runs shellcheck over hack/automation and hack/migration. Fixes the one warning it found in port-pr.sh (an unquoted command substitution passed to git add). Signed-off-by: Roland Huß <rhuss@redhat.com> * test: E2E tests for Go SDK Signed-off-by: Jiri Petrlik <jpetrlik@redhat.com> * test(e2e): use CreateOption for sandbox labels The suite from NVIDIA/OpenShell#3338 predates the switch of Sandboxes().Create from a labels map to CreateOption values. Pass labels through v1.WithLabels and drop the nil maps. Signed-off-by: Roland Huß <rhuss@redhat.com> * feat(hack): run the published gateway for E2E from core's compose files start-gateway.sh fetches deploy/docker/docker-compose.yml and gateway.toml from core at the given tag, pins the supervisor and sandbox images to the same version, starts the gateway with the Docker driver, waits for /readyz, and registers it as gateway 'e2e' for the SDK. It first waits up to 15 minutes for the gateway image of a fresh tag. stop-gateway.sh saves the gateway log and removes the gateway and the sandbox containers it created. Signed-off-by: Roland Huß <rhuss@redhat.com> * ci: run the E2E suite against the published gateway e2e.yml starts the gateway for a core version (default the pin), runs mise run e2e, and uploads the gateway log on failure. ci.yml calls it for pull requests that change the pin, the bindings, the SDK code, the suite, or the gateway scripts. Signed-off-by: Roland Huß <rhuss@redhat.com> * fix(hack): make the E2E gateway's health port reachable Core's gateway.toml binds the health server to 127.0.0.1 inside the container, so the published port 8081 never answers and the readiness wait times out. Bind it to all container interfaces; the compose file publishes it on the host's loopback only. Signed-off-by: Roland Huß <rhuss@redhat.com> * test(e2e): import the provider profiles the suite uses Since NVIDIA/OpenShell#3383 the gateway ships no provider profiles, so creating a claude-code or nvidia provider fails until the profile is imported. TestMain now imports both into the default workspace, based on core's example profiles, replacing earlier copies so reruns start from the same definitions. Signed-off-by: Roland Huß <rhuss@redhat.com> * test(e2e): import the fixture profiles with plain endpoints The gateway rejects an endpoint protocol without access or rules, and the SDK's NetworkEndpoint can't express those yet. The tests only need the credentials, so the fixtures use host and port only. Signed-off-by: Roland Huß <rhuss@redhat.com> * test(e2e): import the fixture profiles without endpoints A credentialed L4 endpoint also needs allow_uninspected_credentials, which NetworkEndpoint can't express either. The tests only exercise credentials, so the fixtures declare no endpoints. Signed-off-by: Roland Huß <rhuss@redhat.com> * test(e2e): import the fixture profiles globally A provider without a profile workspace resolves its type against global profiles, so profiles imported into the default workspace were not found. Import and delete the fixtures in the global scope, as core's E2E harness does with --global. Signed-off-by: Roland Huß <rhuss@redhat.com> * test(e2e): bind provider credentials in the sandbox policy The fixture profiles declare no endpoints, and the gateway withholds credentials of endpointless profiles unless the sandbox policy binds them to an endpoint. The credential tests now create their sandboxes with such a binding, as core's E2E does. Also drops the leftover nil CreateOption arguments. Signed-off-by: Roland Huß <rhuss@redhat.com> * test(e2e): wait for sandbox deletion before deleting providers A sandbox whose policy binds a provider's credentials can't detach it, so deleting the provider right after the sandbox fails with a conflict until the asynchronous sandbox deletion ends. Retry on conflict. Skip the attach/detach test, which needs a profile with endpoints that the SDK can't import yet. Signed-off-by: Roland Huß <rhuss@redhat.com> * test(e2e): reference the profile endpoint issue in the workarounds Signed-off-by: Roland Huß <rhuss@redhat.com> * ci: mint bot tokens from the sdk-bot environment with the client ID Per the App setup on #6: the App is ai-openshell-go-sdk, its secrets are SDK_BOT_CLIENT_ID and SDK_BOT_PRIVATE_KEY (app-id is deprecated in create-github-app-token v3.3.0), and they live in the sdk-bot environment, which only main can deploy to. As repository secrets, any pushed branch could mint a bot token, push to a PR as the bot, and then approve it. core-watch, release-tag, and core-drift run from main and now use environment: sdk-bot. Signed-off-by: Roland Huß <rhuss@redhat.com> * docs(automation): drop references to the deferred adaptation workflow adapt.yml, adapt-context.sh, and the agent:adapt label are not part of this PR: a label trigger runs in PR context, which the sdk-bot environment restricted to main rules out, so the adaptation workflow gets its own design later. Signed-off-by: Roland Huß <rhuss@redhat.com> * test(e2e): close the shared client with a deferred call TestMain now delegates to run(m), so the shared client is closed by one deferred call instead of before every os.Exit. Signed-off-by: Roland Huß <rhuss@redhat.com> * test(e2e): rename requireClient to clientOrSkip The helper skips the test when no gateway is configured; it doesn't assert. The require prefix stays with the helpers that fail the test. Signed-off-by: Roland Huß <rhuss@redhat.com> * fix(hack): never reopen a closed proposal, and stop on API errors open-bump-pr.sh skipped a tag only when its branch existed or a PR from it was merged. A proposal closed by a maintainer, or closed as superseded by a newer pre-release (which deletes the branch), came back on the next hourly run. Skip the tag when any PR from the branch exists. The core tag lookup treated every failure as a deleted tag and closed the open proposal. Only a 404 means the tag is gone now; other errors fail the run, which reports to the drift issue. Signed-off-by: Roland Huß <rhuss@redhat.com> * fix(hack): finish half-done releases and see pin changes behind the tip tag-release.sh created the tag and then the release. When the release step failed, a re-run saw the tag at the commit and stopped, so the release was never created. Check the tag and the release separately and create whichever is missing; API errors other than 404 fail the run. It also decided by diffing only the pushed commit against its parent, so a push of several commits or a rebase merge could miss a pin change that was not the tip. The push workflow now passes the commit before the push as --since, and the script compares the pins at both commits. Signed-off-by: Roland Huß <rhuss@redhat.com> * ci: run E2E when its workflow, tools, or dependencies change The path filter now also matches .github/workflows/e2e.yml, mise.toml, go.mod, and go.sum. Signed-off-by: Roland Huß <rhuss@redhat.com> * fix(hack): check the sandbox image pin and clean up the E2E registration start-gateway.sh verified only the supervisor image pin; it now checks the sandbox image pin too. stop-gateway.sh removes the "e2e" gateway registration and the active-gateway pointer when it still names it. The header no longer claims stop undoes everything and says the scripts are for throwaway CI hosts, since the sandbox cleanup matches core's default namespace label. Signed-off-by: Roland Huß <rhuss@redhat.com> * docs(automation): name the App ai-openshell-sdk-go The App was registered as ai-openshell-sdk-go, following the repository naming convention. Signed-off-by: Roland Huß <rhuss@redhat.com> --------- Signed-off-by: Roland Huß <rhuss@redhat.com> Signed-off-by: Jiri Petrlik <jpetrlik@redhat.com> Co-authored-by: Jiri Petrlik <jpetrlik@redhat.com>
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