Fix follow-up e2e review issues

Author: robzolkos

Makefile

@@ -4,13 +4,14 @@
 	surface-snapshot surface-check lint-actions
 
 BINARY := $(CURDIR)/bin/fizzy
+FIZZY_TEST_BINARY ?= $(BINARY)
 
 # Load local test credentials if present, but refuse tracked local secret files.
 ifneq ($(shell git ls-files --error-unmatch .env.test >/dev/null 2>&1 && echo tracked),)
 $(error .env.test is tracked by Git. Remove it from version control and keep local secret files untracked)
 endif
 -include .env.test
-export FIZZY_TEST_TOKEN FIZZY_TEST_ACCOUNT FIZZY_TEST_API_URL
+export FIZZY_TEST_TOKEN FIZZY_TEST_ACCOUNT FIZZY_TEST_API_URL FIZZY_TEST_BINARY
 VERSION ?= $(shell git describe --tags --always --dirty 2>/dev/null || echo dev)
 LDFLAGS := -X main.version=$(VERSION)
 
@@ -95,7 +96,7 @@ test-unit: check-toolchain
 e2e: build
 	@if [ -z "$$FIZZY_TEST_TOKEN" ]; then echo "Error: FIZZY_TEST_TOKEN not set"; exit 1; fi
 	@if [ -z "$$FIZZY_TEST_ACCOUNT" ]; then echo "Error: FIZZY_TEST_ACCOUNT not set"; exit 1; fi
-	FIZZY_TEST_BINARY=$(BINARY) go test -v -timeout 10m ./e2e/cli_tests/...
+	go test -v -timeout 10m ./e2e/cli_tests/...
 
 test-e2e: e2e
 
@@ -107,7 +108,7 @@ e2e-file: build
 	@if [ -z "$(FILE)" ]; then echo "Usage: make e2e-file FILE=crud_board"; exit 1; fi
 	@if [ -z "$$FIZZY_TEST_TOKEN" ]; then echo "Error: FIZZY_TEST_TOKEN not set"; exit 1; fi
 	@if [ -z "$$FIZZY_TEST_ACCOUNT" ]; then echo "Error: FIZZY_TEST_ACCOUNT not set"; exit 1; fi
-	FIZZY_TEST_BINARY=$(BINARY) go test -v ./e2e/cli_tests/$(FILE)_test.go
+	go test -v ./e2e/cli_tests/$(FILE)_test.go
 
 test-file: e2e-file
 
@@ -116,7 +117,7 @@ e2e-run: build
 	@if [ -z "$(NAME)" ]; then echo "Usage: make e2e-run NAME=TestBoardList"; exit 1; fi
 	@if [ -z "$$FIZZY_TEST_TOKEN" ]; then echo "Error: FIZZY_TEST_TOKEN not set"; exit 1; fi
 	@if [ -z "$$FIZZY_TEST_ACCOUNT" ]; then echo "Error: FIZZY_TEST_ACCOUNT not set"; exit 1; fi
-	FIZZY_TEST_BINARY=$(BINARY) go test -v -run $(NAME) ./e2e/cli_tests/...
+	go test -v -run $(NAME) ./e2e/cli_tests/...
 
 test-run: e2e-run
 

e2e/cli_tests/crud_subresources_test.go

@@ -106,53 +106,59 @@ func TestStepCRUD(t *testing.T) {
 func TestReactionCRUD(t *testing.T) {
 	h := newHarness(t)
 	cardStr := strconv.Itoa(fixture.CardNumber)
-	assertOK(t, h.Run("reaction", "list", "--card", cardStr))
-	assertOK(t, h.Run("reaction", "list", "--card", cardStr, "--comment", fixture.CommentID))
+	cardReactionsBefore := h.Run("reaction", "list", "--card", cardStr)
+	assertOK(t, cardReactionsBefore)
+	commentReactionsBefore := h.Run("reaction", "list", "--card", cardStr, "--comment", fixture.CommentID)
+	assertOK(t, commentReactionsBefore)
 
 	cardReaction := h.Run("reaction", "create", "--card", cardStr, "--content", "+1")
 	assertOK(t, cardReaction)
-	cardReactionID := cardReaction.GetIDFromLocation()
-	if cardReactionID == "" {
-		cardReactionID = cardReaction.GetDataString("id")
-	}
-	if cardReactionID == "" {
-		t.Fatal("no reaction ID in create response")
-	}
-	t.Cleanup(func() { newHarness(t).Run("reaction", "delete", cardReactionID, "--card", cardStr) })
 	cardReactions := h.Run("reaction", "list", "--card", cardStr)
 	assertOK(t, cardReactions)
+	cardReactionID := listAddedID(cardReactionsBefore.GetDataArray(), cardReactions.GetDataArray())
+	if cardReactionID == "" {
+		t.Fatal("expected created card reaction to appear in list")
+	}
+	t.Cleanup(func() {
+		if cardReactionID != "" {
+			newHarness(t).Run("reaction", "delete", cardReactionID, "--card", cardStr)
+		}
+	})
 	if listMapByID(cardReactions.GetDataArray(), cardReactionID) == nil {
 		t.Fatalf("expected card reaction list to include %q", cardReactionID)
 	}
+	deletedCardReactionID := cardReactionID
 	assertOK(t, h.Run("reaction", "delete", cardReactionID, "--card", cardStr))
+	cardReactionID = ""
 	cardReactions = h.Run("reaction", "list", "--card", cardStr)
 	assertOK(t, cardReactions)
-	if listMapByID(cardReactions.GetDataArray(), cardReactionID) != nil {
-		t.Fatalf("expected deleted card reaction %q to be absent from list", cardReactionID)
+	if listMapByID(cardReactions.GetDataArray(), deletedCardReactionID) != nil {
+		t.Fatalf("expected deleted card reaction %q to be absent from list", deletedCardReactionID)
 	}
 
 	commentReaction := h.Run("reaction", "create", "--card", cardStr, "--comment", fixture.CommentID, "--content", "heart")
 	assertOK(t, commentReaction)
-	commentReactionID := commentReaction.GetIDFromLocation()
-	if commentReactionID == "" {
-		commentReactionID = commentReaction.GetDataString("id")
-	}
+	commentReactions := h.Run("reaction", "list", "--card", cardStr, "--comment", fixture.CommentID)
+	assertOK(t, commentReactions)
+	commentReactionID := listAddedID(commentReactionsBefore.GetDataArray(), commentReactions.GetDataArray())
 	if commentReactionID == "" {
-		t.Fatal("no comment reaction ID in create response")
+		t.Fatal("expected created comment reaction to appear in list")
 	}
 	t.Cleanup(func() {
-		newHarness(t).Run("reaction", "delete", commentReactionID, "--card", cardStr, "--comment", fixture.CommentID)
+		if commentReactionID != "" {
+			newHarness(t).Run("reaction", "delete", commentReactionID, "--card", cardStr, "--comment", fixture.CommentID)
+		}
 	})
-	commentReactions := h.Run("reaction", "list", "--card", cardStr, "--comment", fixture.CommentID)
-	assertOK(t, commentReactions)
 	if listMapByID(commentReactions.GetDataArray(), commentReactionID) == nil {
 		t.Fatalf("expected comment reaction list to include %q", commentReactionID)
 	}
+	deletedCommentReactionID := commentReactionID
 	assertOK(t, h.Run("reaction", "delete", commentReactionID, "--card", cardStr, "--comment", fixture.CommentID))
+	commentReactionID = ""
 	commentReactions = h.Run("reaction", "list", "--card", cardStr, "--comment", fixture.CommentID)
 	assertOK(t, commentReactions)
-	if listMapByID(commentReactions.GetDataArray(), commentReactionID) != nil {
-		t.Fatalf("expected deleted comment reaction %q to be absent from list", commentReactionID)
+	if listMapByID(commentReactions.GetDataArray(), deletedCommentReactionID) != nil {
+		t.Fatalf("expected deleted comment reaction %q to be absent from list", deletedCommentReactionID)
 	}
 }
 

e2e/cli_tests/helpers_test.go

@@ -189,6 +189,33 @@ func listIndexByID(items []any, id string) int {
 	return -1
 }
 
+func listAddedID(before, after []any) string {
+	seen := make(map[string]struct{}, len(before))
+	for _, item := range before {
+		m := asMap(item)
+		if m == nil {
+			continue
+		}
+		if id := mapValueString(m, "id"); id != "" {
+			seen[id] = struct{}{}
+		}
+	}
+	for _, item := range after {
+		m := asMap(item)
+		if m == nil {
+			continue
+		}
+		id := mapValueString(m, "id")
+		if id == "" {
+			continue
+		}
+		if _, ok := seen[id]; !ok {
+			return id
+		}
+	}
+	return ""
+}
+
 func bodyPlainText(m map[string]any) string {
 	if m == nil {
 		return ""

e2e/cli_tests/output_contract_test.go

@@ -48,8 +48,11 @@ func assertNonJSON(t *testing.T, stdout, flag string) {
 	if s == "" {
 		t.Fatalf("--%s: produced empty output", flag)
 	}
-	if strings.HasPrefix(s, `{"ok"`) {
-		t.Fatalf("--%s: output looks like a JSON envelope", flag)
+	var obj map[string]any
+	if err := json.Unmarshal([]byte(s), &obj); err == nil {
+		if _, hasOK := obj["ok"]; hasOK {
+			t.Fatalf("--%s: output looks like a JSON envelope", flag)
+		}
 	}
 }
 

e2e/harness/fixture.go

@@ -42,7 +42,9 @@ func NewSharedFixture(cfg *Config) (*SharedFixture, error) {
 	}
 	f := &SharedFixture{cfg: cfg, configHome: tmpDir}
 	if err := f.setup(); err != nil {
-		_ = os.RemoveAll(tmpDir)
+		if teardownErr := f.Teardown(); teardownErr != nil {
+			return nil, fmt.Errorf("%w\ncleanup: %v", err, teardownErr)
+		}
 		return nil, err
 	}
 	return f, nil