feat: add integration tests and full workflow validation by omattsson · Pull Request #19 · omattsson/stackctl

omattsson · 2026-03-28T15:18:04Z

Summary

Adds comprehensive integration tests, edge case tests, and E2E quiet piping validation for stackctl.

New Files

File	Purpose
`cli/test/live/live_test.go`	Live backend integration test (`//go:build live`) — full 10-step lifecycle + bulk operations
`cli/test/integration/edge_case_integration_test.go`	Edge case tests: expired tokens, network errors, invalid input, concurrent ops, large responses, HTTP error codes

Modified Files

File	Change
`cli/test/e2e/cli_e2e_test.go`	Added `--quiet` piping workflow + output format validation tests

Test Coverage

pkg/: 90.2%
cmd/: 91.1%
All tests pass: go test ./... -count=1

Issue #9 Checklist

Integration test script: login → instantiate → deploy → status → overrides → redeploy → logs → stop → clean →- [x] Integration test script: login → in90- [x] Integration test script: login → instantiate → deploy → , i- [x] Integration trre- [x] Integration test script: login → instantiate → deploy → status → overrides → re
cd clicd cliACKCTL_LIcd clic=admin STACKCTL_LIVE_PASS=secret go test -tags live ./test/live/ -v


Closes #9

- Live backend integration test (build-tagged //go:build live): full 10-step lifecycle + bulk operations with t.Cleanup - Edge case tests: expired tokens, network errors, invalid input, concurrent operations, large responses, HTTP error codes - E2E quiet piping workflow + output format validation - Coverage: pkg/ 90.2%, cmd/ 91.1%

Copilot

Pull request overview

Adds broader workflow validation for stackctl by introducing live-backend lifecycle tests, edge-case integration tests (using httptest), and new E2E coverage for --quiet ID-only output used in piping/bulk workflows (Issue #9).

Changes:

Added //go:build live integration tests that exercise a full stack lifecycle plus bulk operations against a real backend.
Added edge-case integration tests covering auth failures, network failures, invalid inputs, concurrency, large responses, and HTTP status→message mapping.
Extended E2E suite to validate --quiet output formatting and its compatibility with bulk operations workflows.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
`cli/test/live/live_test.go`	New live-backend tests for end-to-end lifecycle + bulk operations under `-tags live`.
`cli/test/integration/edge_case_integration_test.go`	New `httptest`-based edge-case integration coverage (401, network errors, invalid inputs, concurrency, large payloads, status mappings).
`cli/test/e2e/cli_e2e_test.go`	Adds E2E assertions for `--quiet` ID-only output and a “quiet→bulk” workflow validation using a mock server.

Copilot · 2026-03-28T15:22:44Z

+// ---------- Server Error Retry Behavior ----------
+
+func TestEdgeCase_ServerErrorRetryBehavior(t *testing.T) {


The test name suggests retry behavior, but the test only validates user-facing error message mapping for various HTTP status codes and does not exercise any retry logic. Consider renaming this test (and section header) to reflect what it actually verifies, or add assertions that confirm retry/backoff behavior if that’s intended.

Suggested change

// ---------- Server Error Retry Behavior ----------

func TestEdgeCase_ServerErrorRetryBehavior(t *testing.T) {

// ---------- Server Error User-Facing Messages ----------

func TestEdgeCase_ServerErrorUserFacingMessages(t *testing.T) {

Copilot · 2026-03-28T15:22:45Z

+	// Quick connectivity check — skip (not fail) if backend is down.
+	httpC := &http.Client{Timeout: 5 * time.Second}
+	resp, err := httpC.Get(baseURL() + "/api/v1/health")
+	if err != nil {
+		t.Skipf("Backend unreachable at %s: %v", baseURL(), err)
+	}
+	resp.Body.Close()
+


The live connectivity check only skips on request errors, but it will proceed even if /api/v1/health returns a non-2xx status (e.g. backend reachable but unhealthy), which can cause confusing downstream failures. Consider checking resp.StatusCode (and optionally reading the body) and skipping when it’s not OK.

Copilot · 2026-03-28T15:22:45Z

+	// Step 6: Set overrides — value override (replicas=2) and branch override on chart ID 1
+	t.Log("Step 6: Set overrides")
+	_, err = c.SetValueOverride(instance.ID, 1, &types.SetValueOverrideRequest{
+		Values: map[string]interface{}{"replicas": 2},
+	})
+	require.NoError(t, err, "set value override")
+
+	_, err = c.SetBranchOverride(instance.ID, 1, &types.SetBranchOverrideRequest{


This test hard-codes chart ID 1 for value/branch overrides. On a live backend, chart IDs may differ between definitions/templates, so this can fail even when stackctl is working correctly. Consider deriving the chart ID from the instantiated template/definition (or querying the instance’s definition/charts) and using that instead of a fixed value.

Suggested change

// Step 6: Set overrides — value override (replicas=2) and branch override on chart ID 1

t.Log("Step 6: Set overrides")

_, err = c.SetValueOverride(instance.ID, 1, &types.SetValueOverrideRequest{

Values: map[string]interface{}{"replicas": 2},

})

require.NoError(t, err, "set value override")

_, err = c.SetBranchOverride(instance.ID, 1, &types.SetBranchOverrideRequest{

// Derive chart ID from the stack status instead of assuming it is always 1.

require.NotEmpty(t, status.Charts, "status must include at least one chart")

chartID := status.Charts[0].ID

// Step 6: Set overrides — value override (replicas=2) and branch override on the derived chart ID

t.Log("Step 6: Set overrides")

_, err = c.SetValueOverride(instance.ID, chartID, &types.SetValueOverrideRequest{

Values: map[string]interface{}{"replicas": 2},

})

require.NoError(t, err, "set value override")

_, err = c.SetBranchOverride(instance.ID, chartID, &types.SetBranchOverrideRequest{

Copilot · 2026-03-28T15:22:45Z

+	// We need at least one definition. Use definition_id=1.
+	const defID uint = 1
+
+	// Step 2: Create 3 stack instances
+	t.Log("Step 2: Create 3 stack instances")
+	var ids []uint
+	for i := 0; i < 3; i++ {
+		inst, err := c.CreateStack(&types.CreateStackRequest{
+			Name:              fmt.Sprintf("live-bulk-%d-%d", time.Now().UnixMilli(), i),
+			StackDefinitionID: defID,
+			Branch:            "main",
+		})


This live test assumes stack definition ID 1 exists. That makes the test environment-dependent and likely to fail on fresh or differently-seeded backends. Consider listing definitions (or templates) and selecting an available ID dynamically instead of hard-coding 1.

Copilot · 2026-03-28T15:22:45Z

+			t.Parallel()
+			err := tt.fn()
+			require.Error(t, err, "should return an error, not panic")
+			assert.Contains(t, err.Error(), "connect")


These assertions rely on err.Error() containing the substring "connect", but the exact wording of network errors varies across platforms and Go versions (and the client wraps errors with "making request: ..."). To reduce test flakiness, consider asserting on error types via errors.As (e.g., *url.Error / net.OpError) or checking a stable prefix like "making request:" instead of matching OS-dependent text.

Suggested change

assert.Contains(t, err.Error(), "connect")

assert.ErrorContains(t, err, "making request:")

- Rename TestEdgeCase_ServerErrorRetryBehavior to ServerErrorUserFacingMessages - Check HTTP status code in live test connectivity check, not just errors - Derive chart ID from template instead of hardcoding 1 - Discover definitions dynamically in bulk test instead of assuming ID 1 - Use 'making request:' assertion for network errors (platform-independent)

Add 13 negative tests covering: - HTTP 401/403/500 error handling per command group (stack, template, definition, cluster, git, bulk) - Empty token response from server (login) - Invalid JSON file import (definition) - Empty context name validation (config) - Malformed JSON and empty response body handling (client)

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Copilot · 2026-03-28T16:35:08Z

+	// Derive chart ID from the template instead of hardcoding.
+	var chartID uint = 1
+	if len(tmpl.Charts) > 0 {
+		chartID = tmpl.Charts[0].ID
+	}
+
+	// Step 6: Set overrides — value override (replicas=2) and branch override
+	t.Log("Step 6: Set overrides")
+	_, err = c.SetValueOverride(instance.ID, chartID, &types.SetValueOverrideRequest{
+		Values: map[string]interface{}{"replicas": 2},
+	})
+	require.NoError(t, err, "set value override")
+
+	_, err = c.SetBranchOverride(instance.ID, chartID, &types.SetBranchOverrideRequest{
+		Branch: "feature/test",
+	})
+	require.NoError(t, err, "set branch override")


chartID falls back to 1 when the selected template has no charts. That will make the override calls target a potentially non-existent chart and fail the live workflow in environments where templates can be chart-less. Consider skipping the override steps (or the whole test) when len(tmpl.Charts)==0, rather than assuming chart ID 1 exists.

Copilot · 2026-03-28T16:35:08Z

+	// The login command does not validate empty tokens — it saves and reports success.
+	// Verify the token file was written with an empty token value.
+	require.NoError(t, err)
+	assert.Contains(t, buf.String(), "Logged in as test")
+
+	tokenPath := filepath.Join(os.Getenv("STACKCTL_CONFIG_DIR"), "tokens", "test.json")
+	data, readErr := os.ReadFile(tokenPath)
+	require.NoError(t, readErr)
+
+	var stored storedToken
+	require.NoError(t, json.Unmarshal(data, &stored))
+	assert.Empty(t, stored.Token, "token should be empty when server returns empty token")


This test codifies that stackctl login succeeds even when the server returns an empty JWT token, and it verifies that an empty token is written to disk. That behavior will leave users in a broken “logged in” state and makes it harder to safely tighten validation later. Consider changing the command to treat an empty token as an error (and update this test to expect a failure and no token file written).

Suggested change

// The login command does not validate empty tokens — it saves and reports success.

// Verify the token file was written with an empty token value.

require.NoError(t, err)

assert.Contains(t, buf.String(), "Logged in as test")

tokenPath := filepath.Join(os.Getenv("STACKCTL_CONFIG_DIR"), "tokens", "test.json")

data, readErr := os.ReadFile(tokenPath)

require.NoError(t, readErr)

var stored storedToken

require.NoError(t, json.Unmarshal(data, &stored))

assert.Empty(t, stored.Token, "token should be empty when server returns empty token")

// The login command should treat an empty token as an error and avoid writing a token file.

require.Error(t, err, "login should fail when server returns an empty token")

tokenPath := filepath.Join(os.Getenv("STACKCTL_CONFIG_DIR"), "tokens", "test.json")

_, statErr := os.Stat(tokenPath)

assert.True(t, os.IsNotExist(statErr), "token file should not exist when server returns empty token")

- Skip override steps in live test when template has no charts instead of falling back to hardcoded chart ID 1 - Add empty token validation in login command — server returning an empty JWT now results in an error instead of silently saving a broken token file - Update TestLoginCmd_EmptyTokenFromServer to expect failure

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Copilot · 2026-03-28T16:45:12Z

+func TestDefinitionImportCmd_InvalidJSON(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		t.Fatal("API should not be called for invalid JSON content")
+	}))
+	defer server.Close()
+
+	tmpDir := t.TempDir()
+	filePath := filepath.Join(tmpDir, "bad.json")
+	require.NoError(t, os.WriteFile(filePath, []byte(`{invalid json}`), 0644))
+
+	_ = setupStackTestCmd(t, server.URL)
+
+	definitionImportCmd.Flags().Set("file", filePath)
+	t.Cleanup(func() {
+		definitionImportCmd.Flags().Set("file", "")
+	})
+
+	err := definitionImportCmd.RunE(definitionImportCmd, []string{})
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "marshaling request body")
+}


TestDefinitionImportCmd_InvalidJSON assumes invalid JSON should be rejected before any API call (the handler t.Fatals if called), but definitionImportCmd passes the file bytes directly to c.ImportDefinition(data). client.ImportDefinition wraps the bytes in json.RawMessage and json.Marshal does not validate the JSON, so the request will still be sent and this test will fail. Either add a json.Valid(data) check in the import command/client and return a clear "invalid JSON" error, or update the test to expect a server-side 400/validation error instead of asserting the API is not called.

Add json.Valid check in definition import command so malformed JSON files are rejected client-side with a clear error message instead of being sent to the server. Update test assertion.

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Copilot · 2026-03-28T16:51:07Z

+	// Create a server and immediately close it to get a connection-refused endpoint
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
+	closedURL := server.URL
+	server.Close()
+
+	c := client.New(closedURL)


TestEdgeCase_NetworkErrors uses a URL from an httptest.NewServer that is immediately closed to simulate a network failure. This can be flaky under parallel test execution because the OS can reuse that port for another listener, making the request unexpectedly succeed. Prefer a deterministic failure (e.g., a custom RoundTripper that returns an error, or an unroutable address like 192.0.2.1, or setting c.HTTPClient.Transport to a stub).

Copilot · 2026-03-28T16:51:07Z

+		// Bulk deploy
+		case r.URL.Path == "/api/v1/stack-instances/bulk/deploy" && r.Method == http.MethodPost:
+			var req map[string]interface{}
+			json.NewDecoder(r.Body).Decode(&req)
+			w.WriteHeader(http.StatusOK)
+			json.NewEncoder(w).Encode(map[string]interface{}{
+				"results": []map[string]interface{}{
+					{"id": 1, "success": true},
+					{"id": 2, "success": true},
+					{"id": 3, "success": true},
+				},
+			})
+
+		// Bulk stop
+		case r.URL.Path == "/api/v1/stack-instances/bulk/stop" && r.Method == http.MethodPost:
+			var req map[string]interface{}
+			json.NewDecoder(r.Body).Decode(&req)
+			w.WriteHeader(http.StatusOK)
+			json.NewEncoder(w).Encode(map[string]interface{}{
+				"results": []map[string]interface{}{
+					{"id": 1, "success": true},
+					{"id": 2, "success": true},
+					{"id": 3, "success": true},
+				},
+			})
+
+		// Bulk delete
+		case r.URL.Path == "/api/v1/stack-instances/bulk/delete" && r.Method == http.MethodPost:
+			var req map[string]interface{}
+			json.NewDecoder(r.Body).Decode(&req)
+			w.WriteHeader(http.StatusOK)
+			json.NewEncoder(w).Encode(map[string]interface{}{


startE2EQuietPipingMockServer decodes the bulk request body but never validates it (and ignores decode errors). As written, the quiet piping test can pass even if the CLI sends the wrong payload or no IDs at all. Consider asserting the JSON body contains the expected IDs (and failing the handler/test on decode mismatch) to ensure the workflow is actually validated end-to-end.

Copilot · 2026-03-28T16:51:08Z

+	for _, line := range lines {
+		line = strings.TrimSpace(line)
+		assert.Regexp(t, `^\d+$`, line, "each line should be a numeric ID only")
+	}
+
+	// 3. Run bulk deploy --ids 1,2,3 → verify success
+	stdout, _, err = runStackctl(t, dir, "bulk", "deploy", "--ids", "1,2,3")
+	require.NoError(t, err)
+	assert.Contains(t, stdout, "1")
+	assert.Contains(t, stdout, "2")
+	assert.Contains(t, stdout, "3")
+
+	// 4. Run bulk stop --ids 1,2,3 → verify success
+	stdout, _, err = runStackctl(t, dir, "bulk", "stop", "--ids", "1,2,3")
+	require.NoError(t, err)
+	assert.Contains(t, stdout, "1")
+	assert.Contains(t, stdout, "2")
+	assert.Contains(t, stdout, "3")
+
+	// 5. Run bulk delete --ids 1,2,3 --yes → verify success (needs --yes to skip confirmation)
+	stdout, _, err = runStackctl(t, dir, "bulk", "delete", "--ids", "1,2,3", "--yes")
+	require.NoError(t, err)
+	assert.Contains(t, stdout, "1")
+	assert.Contains(t, stdout, "2")
+	assert.Contains(t, stdout, "3")


TestE2E_QuietPipingWorkflow hard-codes --ids 1,2,3 for the bulk commands instead of deriving the IDs from the preceding stack list --quiet output. This means the test validates the quiet output format and the bulk commands independently, but not the actual list→bulk piping workflow. Building the --ids argument from the captured quiet output would make this a true workflow test.

Suggested change

for _, line := range lines {

line = strings.TrimSpace(line)

assert.Regexp(t, `^\d+$`, line, "each line should be a numeric ID only")

}

// 3. Run bulk deploy --ids 1,2,3 → verify success

stdout, _, err = runStackctl(t, dir, "bulk", "deploy", "--ids", "1,2,3")

require.NoError(t, err)

assert.Contains(t, stdout, "1")

assert.Contains(t, stdout, "2")

assert.Contains(t, stdout, "3")

// 4. Run bulk stop --ids 1,2,3 → verify success

stdout, _, err = runStackctl(t, dir, "bulk", "stop", "--ids", "1,2,3")

require.NoError(t, err)

assert.Contains(t, stdout, "1")

assert.Contains(t, stdout, "2")

assert.Contains(t, stdout, "3")

// 5. Run bulk delete --ids 1,2,3 --yes → verify success (needs --yes to skip confirmation)

stdout, _, err = runStackctl(t, dir, "bulk", "delete", "--ids", "1,2,3", "--yes")

require.NoError(t, err)

assert.Contains(t, stdout, "1")

assert.Contains(t, stdout, "2")

assert.Contains(t, stdout, "3")

for i, line := range lines {

lines[i] = strings.TrimSpace(line)

assert.Regexp(t, `^\d+$`, lines[i], "each line should be a numeric ID only")

}

// 3. Build the --ids argument from the quiet output to simulate piping workflow

idsCSV := strings.Join(lines, ",")

// 4. Run bulk deploy --ids <derived> → verify success

stdout, _, err = runStackctl(t, dir, "bulk", "deploy", "--ids", idsCSV)

require.NoError(t, err)

for _, id := range lines {

assert.Contains(t, stdout, id)

}

// 5. Run bulk stop --ids <derived> → verify success

stdout, _, err = runStackctl(t, dir, "bulk", "stop", "--ids", idsCSV)

require.NoError(t, err)

for _, id := range lines {

assert.Contains(t, stdout, id)

}

// 6. Run bulk delete --ids <derived> --yes → verify success (needs --yes to skip confirmation)

stdout, _, err = runStackctl(t, dir, "bulk", "delete", "--ids", idsCSV, "--yes")

require.NoError(t, err)

for _, id := range lines {

assert.Contains(t, stdout, id)

}

- Use custom RoundTripper for network error tests instead of closing an httptest server (avoids port-reuse flakiness) - Validate bulk request bodies in e2e mock server — return 400 if IDs are missing or body fails to decode - Derive --ids from quiet output in e2e piping workflow test instead of hardcoding, making it a true list→bulk pipe test

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Copilot · 2026-03-28T17:00:29Z

+//go:build live
+


Because this directory contains only a //go:build live file, running go test ./... without -tags live will likely fail with build constraints exclude all Go files for cli/test/live. Consider adding an untagged (or !live-tagged) stub file in this package (e.g., a doc-only doc.go/*_test.go) so the package remains buildable when the live tag is not set, while still keeping the actual tests behind the live tag.

Suggested change

//go:build live

Copilot · 2026-03-28T17:00:29Z

+		_, err = c.SetBranchOverride(instance.ID, chartID, &types.SetBranchOverrideRequest{
+			Branch: "feature/test",
+		})
+		require.NoError(t, err, "set branch override")
+
+		// Verify overrides were persisted
+		valOverrides, err := c.ListValueOverrides(instance.ID)
+		require.NoError(t, err, "list value overrides")
+		assert.NotEmpty(t, valOverrides, "should have at least one value override")
+
+		branchOverrides, err := c.ListBranchOverrides(instance.ID)
+		require.NoError(t, err, "list branch overrides")
+		assert.NotEmpty(t, branchOverrides, "should have at least one branch override")


The live workflow test hard-codes a branch override to feature/test. This makes the live test brittle across environments (the repo behind the template/chart may not have that branch, causing an otherwise healthy backend to fail the test). Consider using a branch that’s guaranteed to exist (e.g., main/default), or make the override branch configurable via an env var (and skip the override step if unset).

Suggested change

_, err = c.SetBranchOverride(instance.ID, chartID, &types.SetBranchOverrideRequest{

Branch: "feature/test",

})

require.NoError(t, err, "set branch override")

// Verify overrides were persisted

valOverrides, err := c.ListValueOverrides(instance.ID)

require.NoError(t, err, "list value overrides")

assert.NotEmpty(t, valOverrides, "should have at least one value override")

branchOverrides, err := c.ListBranchOverrides(instance.ID)

require.NoError(t, err, "list branch overrides")

assert.NotEmpty(t, branchOverrides, "should have at least one branch override")

branch := os.Getenv("STACKCTL_LIVE_BRANCH")

if branch == "" {

t.Log("Step 6 (branch override): Skipped — STACKCTL_LIVE_BRANCH not set")

} else {

_, err = c.SetBranchOverride(instance.ID, chartID, &types.SetBranchOverrideRequest{

Branch: branch,

})

require.NoError(t, err, "set branch override")

}

// Verify overrides were persisted

valOverrides, err := c.ListValueOverrides(instance.ID)

require.NoError(t, err, "list value overrides")

assert.NotEmpty(t, valOverrides, "should have at least one value override")

if branch != "" {

branchOverrides, err := c.ListBranchOverrides(instance.ID)

require.NoError(t, err, "list branch overrides")

assert.NotEmpty(t, branchOverrides, "should have at least one branch override")

}

- Add doc.go stub in test/live/ so the package builds without the live tag (go test ./... no longer fails) - Make branch override configurable via STACKCTL_LIVE_BRANCH env var; skip the branch override step when unset to avoid failures in environments lacking the test branch

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Copilot · 2026-03-28T17:07:33Z

@@ -0,0 +1,6 @@
+// Package live contains integration tests that run against a live backend.
+// These tests are gated behind the "live" build tag and require environment
+// variables STACKCTL_LIVE_URL, STACKCTL_LIVE_USER, and STACKCTL_LIVE_PASS.


The package doc says STACKCTL_LIVE_URL is required, but baseURL() falls back to http://localhost:8081 when it’s unset. Adjust the comment to say STACKCTL_LIVE_URL is optional (or enforce it in code).

Suggested change

// variables STACKCTL_LIVE_URL, STACKCTL_LIVE_USER, and STACKCTL_LIVE_PASS.

// variables STACKCTL_LIVE_USER and STACKCTL_LIVE_PASS. STACKCTL_LIVE_URL is

// optional; if unset, the tests default to http://localhost:8081.

Copilot AI review requested due to automatic review settings March 28, 2026 15:18

Copilot started reviewing on behalf of omattsson March 28, 2026 15:18 View session