Skip to content

test: retire stale connections scenario tests (command removed in #221) - #246

Merged
eddietejeda merged 1 commit into
mainfrom
chore/retire-connections-scenario-tests
Aug 3, 2026
Merged

test: retire stale connections scenario tests (command removed in #221)#246
eddietejeda merged 1 commit into
mainfrom
chore/retire-connections-scenario-tests

Conversation

@eddietejeda

Copy link
Copy Markdown
Contributor

What

Retire two scenario tests that call the hotdata connections command removed in #221 ("scope all commands to active database; remove connections command").

Background

#221 deleted the connections command but left these tests behind (and never updated the server-side scenario manifest). They've been silently broken since:

  • tests/connections_read.rs — calls the removed connections list / connections <id>. It exists only to satisfy scenario-parity's file-existence check and skips in CI (HOTDATA_SDK_TEST_CONNECTION_ID is unset), so nobody noticed it was dead. The scenario has no current CLI surface (connections are resolved internally now). Deleted.
  • tests/auth_unknown_workspace.rs — a security scenario: a fabricated workspace id must be rejected with no cross-workspace leak. It also called connections list, so it exited non-zero at arg-parse — i.e. it was passing for the wrong reason and never actually reached the server to verify the rejection. Switched to databases list, a real workspace-scoped listing that hits the gateway with the fabricated workspace id, restoring genuine coverage. (The scenario's ops: ["*"] allows any op.)

Dependency / merge order

Companion PR hotdata-dev/www.hotdata.dev#290 adds cli to optional_for on the connections_read scenario. That must merge first — until it does, scenario-parity fetches the live manifest (which still requires the CLI to have tests/connections_read.rs) and this PR's scenario-parity check will be red. Once #290 is on main, re-run and it goes green.

Verification

  • cargo test + cargo fmt --check pass locally; auth_unknown_workspace compiles and skips cleanly without creds.
  • No remaining references to connections list outside an explanatory comment.

The `hotdata connections` command was removed in #221 (all commands scoped to the
active database), but two scenario tests kept calling it:

- `tests/connections_read.rs` — calls the removed `connections list`/`<id>`. It
  only existed to satisfy scenario-parity's file-existence check and skips in CI
  (no `HOTDATA_SDK_TEST_CONNECTION_ID`), so it was dead. The scenario has no CLI
  equivalent; a companion www.hotdata.dev PR opts the CLI out of it. Deleted.

- `tests/auth_unknown_workspace.rs` — a security scenario (a fabricated workspace
  id must be rejected, no cross-workspace leak). It also called `connections
  list`, so it exited non-zero at *arg-parse* — passing for the wrong reason and
  never actually exercising the server rejection. Switched to `databases list`, a
  real workspace-scoped listing that reaches the gateway with the fabricated
  workspace id, restoring genuine coverage.

Depends on hotdata-dev/www.hotdata.dev#290 (opts the CLI out of connections_read)
merging first, or scenario-parity will still require the deleted file.
@eddietejeda
eddietejeda requested a review from a team as a code owner August 2, 2026 23:11
@eddietejeda
eddietejeda requested review from anoop-narang and removed request for a team August 2, 2026 23:11
Comment thread tests/connections_read.rs

#[test]
fn connections_read() {
let (cli, connection_id) = skip_if_no_connection!();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: (not blocking) this was the only caller of skip_if_no_connection!, so deleting it leaves the whole connection-id scaffolding dead:

  • tests/common/mod.rs:265-292 — the skip_if_no_connection! macro (won't warn, since #[macro_export] macros are exempt from unused_macros)
  • tests/common/mod.rs:48,70TestEnv::connection_id and its load_env wiring (silenced by the crate-level #![allow(dead_code)])
  • .github/workflows/integration-tests.yml:98HOTDATA_SDK_TEST_CONNECTION_ID passed to the integration job

Since connections are resolved internally now and no scenario is expected to re-add a connections CLI surface, dropping these in the same PR keeps the harness honest — otherwise the next reader sees a supported connection-gated path that nothing uses. Fine to leave if you'd rather keep it as a hook for a future scenario.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verified the substantive claim: -w is global = true on Commands::Databases (src/cli.rs:52-58) and resolve_workspace (src/main.rs:74-78) returns an explicit --workspace-id verbatim with no client-side validation, so the fabricated id does reach the gateway as X-Workspace-Id. databases::list exits non-zero via e.exit() on API error (src/commands/databases.rs:943), so the non-zero assertion is real coverage now rather than an arg-parse artifact.

One non-blocking nit inline.

@codecov

codecov Bot commented Aug 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@eddietejeda
eddietejeda merged commit 337dc68 into main Aug 3, 2026
15 of 16 checks passed
@eddietejeda
eddietejeda deleted the chore/retire-connections-scenario-tests branch August 3, 2026 03: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.

1 participant