fix(embeddeddolt): /cmd/bd: tests for additional commands#2842
fix(embeddeddolt): /cmd/bd: tests for additional commands#2842coffeegoddd merged 6 commits intosteveyegge:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds additional embedded-Dolt (build-tagged) integration tests for several bd CLI commands, and ensures certain write commands flush pending changes via CommitPending when running in embedded Dolt mode so changes persist reliably.
Changes:
- Added new
//go:build embeddeddoltsubprocess integration tests for:version,defer/undefer,ship,ready,blocked, andorphans(including concurrent invocations). - Added embedded-mode “flush Dolt commit” behavior (
store.CommitPending) todefer,undefer,ship,rename, and ADO sync (non-dry-run). - Expanded embedded-mode coverage around concurrency behavior for the newly tested commands.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/bd/version_embedded_test.go | Adds embedded integration tests for bd version and concurrent invocation. |
| cmd/bd/defer_embedded_test.go | Adds embedded integration tests (and concurrency) for bd defer, plus shared helpers used by undefer tests. |
| cmd/bd/defer.go | Flushes pending embedded Dolt changes after defer. |
| cmd/bd/undefer_embedded_test.go | Adds embedded integration tests (and concurrency) for bd undefer. |
| cmd/bd/undefer.go | Flushes pending embedded Dolt changes after undefer. |
| cmd/bd/ship_embedded_test.go | Adds embedded integration tests (and concurrency) for bd ship including --force / --dry-run. |
| cmd/bd/ship.go | Flushes pending embedded Dolt changes after a successful ship (non-dry-run, non-already-shipped). |
| cmd/bd/rename.go | Flushes pending embedded Dolt changes after a successful rename. |
| cmd/bd/ready_embedded_test.go | Adds embedded integration tests (and concurrency) for bd ready. |
| cmd/bd/blocked_embedded_test.go | Adds embedded integration tests (and concurrency) for bd blocked. |
| cmd/bd/orphans_embedded_test.go | Adds embedded integration tests (and concurrency) for bd orphans. |
| cmd/bd/ado.go | Flushes pending embedded Dolt changes after ADO sync writes (non-dry-run). |
| s := strings.TrimSpace(string(out)) | ||
| start := strings.IndexAny(s, "[{") | ||
| if start >= 0 { | ||
| if !json.Valid([]byte(s[start:])) { | ||
| t.Errorf("invalid JSON in ready output: %s", s[:min(200, len(s))]) | ||
| } | ||
| } |
There was a problem hiding this comment.
In the --json subtest, the test only validates JSON if it can find a '{' or '[' in the output. If the command accidentally prints non-JSON (or nothing) despite --json, this test will still pass. For --json, assert that JSON is present (start >= 0) and then validate json.Valid on the remainder.
| s := strings.TrimSpace(string(out)) | ||
| start := strings.IndexAny(s, "[{") | ||
| if start >= 0 { | ||
| if !json.Valid([]byte(s[start:])) { | ||
| t.Errorf("invalid JSON in blocked output: %s", s[:min(200, len(s))]) | ||
| } | ||
| } |
There was a problem hiding this comment.
In the --json subtest, the test only validates JSON if it finds a '{' or '['. With --json, lack of any JSON should fail the test (as done in other embedded tests). Consider asserting start >= 0 (or specifically requiring an array/object) before calling json.Valid.
| out := bdOrphans(t, bd, dir, "--json") | ||
| // Should produce valid output without crashing | ||
| if len(strings.TrimSpace(out)) == 0 { | ||
| t.Error("expected non-empty --json output") | ||
| } |
There was a problem hiding this comment.
The --json test currently only checks for non-empty output. Since bd orphans --json calls outputJSON directly, this should assert that the output contains a JSON array/object and that json.Valid succeeds, otherwise the test can pass while emitting non-JSON.
No description provided.