Skip to content

fix(data list): route through the listDatasetsFn seam + cover the query path#248

Merged
saadqbal merged 1 commit into
developfrom
refactor/cli-test-seams
Jul 14, 2026
Merged

fix(data list): route through the listDatasetsFn seam + cover the query path#248
saadqbal merged 1 commit into
developfrom
refactor/cli-test-seams

Conversation

@LukasWodka

@LukasWodka LukasWodka commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Second increment from the CLI test-coverage audit — the data list path. A focused slice of the "test seams" work: fixes a genuine consistency bug in production and makes ListDatasets's query path testable, with the tests.

Production changes

  1. runDataList now routes through the listDatasetsFn seam (data_list.go:118). It was calling push.ListDatasets directly, while the listDatasetsFn seam it should use already exists and is used two lines over by destTableExists / resolveDeleteTarget (data.go:1462). This was a real consistency bug — data list was the only dataset-query caller bypassing the seam — and it left runDataList's success/error paths untestable.

  2. ListDatasets split so its exec path is fakeable. The function built a real SPDYExecutor internally, so the exec + error-wrap + parse logic sat at 0% (a real cluster was the only way in). Extracted listDatasetsWith(ctx, exec Executor, ns, pod, container)ListDatasets still locates the mysql pod and builds the production executor, then delegates. Public signature and the listDatasetsFn seam are unchanged.

Test changes

  • TestListDatasetsWith (new) — drives the split-out core with a fake Executor: a query result is parsed into table names, an empty database yields no datasets, and a failing exec surfaces querying datasets + the remote stderr (via stderrSuffix).
  • fakeExecutor gains a stdoutToReturn field (stream_test.go) — it previously discarded the stdout writer (it was built for tar-streaming, which doesn't read stdout). Now it can return a stdout body, which ListDatasets needs. Existing callers don't set it → no behavior change (all push tests still pass).

Not in this PR (follow-ups)

The heavier seam work stays separate so each PR is reviewable:

  • Routing runClusterInfo / runClusterDoctor / realProbeEnv through loadClusterFn/newClientsetFn, and the teardownFn/stageFn seams — plus the command-path tests they unblock (runDataDelete execute + partial-fail, runClusterInfo post-discovery, runDataList success path). Those interlock with the resolveClusterTarget-success / Bound-PVC fixture the audit flagged, which I'll build next.

Test plan

  • make ci green (build, -race, gofmt, lint, schema).
  • Whole internal/push package re-run to confirm the fakeExecutor change didn't regress the tar-stream / stage / teardown tests.

🤖 Generated with Claude Code


Note

Low Risk
Refactor and test seams only; production behavior for listing datasets is unchanged aside from consistent use of the existing injectable hook.

Overview
tracebloc data list now calls listDatasetsFn instead of push.ListDatasets directly, matching destTableExists / resolveDeleteTarget and enabling CLI tests to stub dataset listing.

In internal/push, ListDatasets still discovers the mysql pod and builds SPDYExecutor, but the information_schema query, error wrapping, and parsing live in injectable listDatasetsWith. TestListDatasetsWith covers success, empty output, and exec failure (including stderr). fakeExecutor can return stdout for those tests without changing tar-stream behavior when stdoutToReturn is unset.

Reviewed by Cursor Bugbot for commit 398b293. Bugbot is set up for automated code reviews on this repo. Configure here.

…ry path

- runDataList called push.ListDatasets directly, bypassing the
  listDatasetsFn seam its sibling dataset-query callers (destTableExists,
  resolveDeleteTarget) already use (data.go:1462). A real consistency bug
  that also left the path untestable. Now routes through the seam.
- Split listDatasetsWith(ctx, exec Executor, …) out of ListDatasets so the
  exec + error-wrap + parse path is fakeable — it was 0% because
  ListDatasets built a real SPDYExecutor internally. Public signature and
  the listDatasetsFn seam are unchanged.
- TestListDatasetsWith: happy parse, empty database, and a failing exec
  surfacing "querying datasets" + the remote stderr. fakeExecutor gains a
  stdoutToReturn field (no impact on the tar-stream callers).

make ci green. Follow-up: the loadClusterFn routing + teardown/stage seams
+ their command-path tests (interlock with the resolveClusterTarget fixture).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 398b293. Configure here.

@LukasWodka LukasWodka self-assigned this Jul 14, 2026

@saadqbal saadqbal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice — good catch that data list was the only dataset-query caller skipping the listDatasetsFn seam; routing it through matches destTableExists/delete and unblocks the tests. Refactor is behavior-preserving. LGTM 👍

@saadqbal
saadqbal merged commit bb56683 into develop Jul 14, 2026
22 checks passed
@saadqbal
saadqbal deleted the refactor/cli-test-seams branch July 14, 2026 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants