Remove low-value local tests and adopt a one-layer-per-behaviour test rule

Read the proposed one-layer-per-behaviour rule, then inspect AGENTS.md, the add-cli-command skill, and tests/cli_request_shape_test.rs. Start with one listed test category and compare its subprocess assertions with the corresponding unit tests before changing anything. Done means the rule is documented and redundant checks are removed or merged without losing coverage; confirm the reviewer estimates as you go.

Summary

A review of the local cargo test -p clickhousectl suite (~2,420 tests; live Cloud integration suites were out of scope) found roughly 380 tests that can be deleted and ~270 that can be merged into table tests, about a quarter of the suite. They cost run time and maintenance but catch nothing the remaining tests don't.

The biggest group (~250 tests) checks the same behaviour twice. The wiremock subprocess tests in tests/cli_request_shape_test.rs re-check invalid-input rejections and request bodies that the clap-parse and build_*_request unit tests already cover.

Test run time is tracked separately; see the linked speed issue. Line numbers are as of f3698472. All counts are reviewer estimates, so confirm each one before deleting.

Proposed rule (add to AGENTS.md and the add-cli-command skill)

Check each behaviour in one place: the unit tests by default.

• Input validation and request-body shape belong in unit tests (clap parse tests, request builders, pure logic), which are about 100× cheaper than a subprocess test.
• Keep a subprocess test only for behaviour that needs the real binary: request order, auth headers, files written, exit codes and the JSON error envelope, stdout/stderr contracts, locks and signals.
• If a subprocess test asserts that something fails before any HTTP request, and clap is what rejects it, the test proves nothing new. Clap exits with code 2 before any handler runs.

Without this rule the pattern will keep coming back.

⚠️ Decide which copy to keep before deleting

Reviewers sometimes proposed deleting both copies of the same check, one in the unit tests and one in the subprocess tests. Deleting both would lose coverage. Settle these with the rule above first:

• CDC scaling: build_cdc_scaling_request_* (unit, clickpipes.rs ~7116/7127) vs the wiremock CDC scaling body tests (~18656/18700)
• Kafka credentials: kafka_credentials_*_shape (unit, clickpipes.rs ~14848–14916) vs the wiremock kafka_*_credentials_shape tests
• Backup config, member update and private-endpoint validation, where deletion was proposed on both sides

Findings by category

A. Tests that check exact wording (repo policy already forbids pinning help or doc wording)

• clickpipes.rs ~9436–9490 and ~11701: 4 tests only check that clap prints --flag <VALUE_NAME> or the possible values in help.
• services.rs the_stdin_conflict_message_says_what_does_work asserts that a const contains its own text.
• services.rs service_query_agent_context_block_stays_within_its_line_budget and postgres.rs postgres_screens_describe_org_id_consistently… re-check rules that the whole-tree tests in src/cli.rs already enforce for every command.
• local/output.rs ~3251–3700: 16 *_display tests pin Display strings. Keep the ones that test layout logic (placeholders, the singular "1 server").
• local/cli.rs server_start_help_renders_default_name_without_escaped_quotes pins help rendering.
• The same full error sentence is pinned in several places:
  • "No such service/org…": client.rs ~1288–1366 plus three per-command subprocess tests.
  • "server not found": pinned word for word in local_server_state_machine_test, local_server_selection_test and local_structured_errors_test.
  • local_client_project_scope_errors_test: 5 tests compare the whole stderr exactly.
  • local_udf_deploy_test: 11 subprocess runs each check an exact message, and the logic is unit-tested in src/udf.rs and src/local/udf.rs.
• Version error wording is pinned three times: spec.rs, local/cli.rs ~1101–1135, and tests/local_version_error_test.rs.
• Keep the src/cli.rs whole-tree help tests; they pin no wording. One exception: local_clients_share_common_argument_order… pins an exact 7-flag list. Assert that the two clients match instead.

B. Tests for removed or absent flags (repo policy forbids these)

• postgres.rs postgres_prometheus_does_not_advertise_unsupported_filters and promote_and_switchover_reject_removed_wait_flags, plus the wiremock test postgres_role_changes_reject_removed_wait_flags_before_requests
• cloud/cli.rs former_positional_org_selectors_are_usage_errors, plus the wiremock tests org_get_and_update_reject_retired_positionals_with_global_selector and global_org_id_retires_all_positional_org_forms_before_http
• api_keys.rs key_create_does_not_accept_clear_expiry, clickstack.rs untargeted_commands_do_not_accept_child_selectors, the wiremock test start_paused_omission_and_database_rejection_are_preserved, and the UnknownArgument halves of a few others

C. Tests of clap, serde or std rather than our code

• clickpipes.rs possible_value_parsers_reject_unknown_values (~235 lines) only tests PossibleValuesParser.
• clickpipes.rs accepted_possible_values_are_pinned compares each const to a literal copy of itself. Rewrite it to deserialize each value into its clickhouse-cloud-api enum; that is the drift its comment says it guards against.
• clickpipes.rs parse_enum_* and cloud/types_test.rs test_activity_* (×6) re-test library serde. Several token, credential and delete-response round-trip tests only exercise the derive.
• telemetry.rs claim_yields_true_exactly_once tests AtomicBool::swap, and init_snapshots_the_executable_path_once tests OnceLock.
• cli_request_shape_test.rs clear_inherited_env_removes_agent_credentials_home_and_path tests Command::env_clear.
• About 40 parse tests in the cloud modules and about 13 in local/cli.rs only show that clap's derive works. Examples: parses_postgres_get, parses_org_balance_command, parses_list_and_get. Nine of the local/cli.rs ones are already covered by the tables that loop over all 10 instance commands.

D. Assertions that can never fail

• main.rs json_output_true_when_flag_set asserts json_output(true).
• cli_request_shape_test.rs ~13150 asserts that "Waiting for the new API key" is absent, but that string exists nowhere in the code.
• stdout.contains("-") always passes because table borders contain -: organizations.rs notifications_table_tolerates_absent_fields and three wiremock table tests (~22069, ~23468, ~23831).
• Two wiremock tests (~17806 and ~17981) assert that human-mode words are absent. The harness doesn't clear the environment, so agent detection can force JSON and those words can never appear; the test's own comment says so.
• Several tests check !status.success() instead of the exact exit code.

E. The same behaviour checked at two layers (~250 tests)

• Invalid-input rejections run as subprocesses: about 50 tests, often looping over many runs. Examples:
  • kinesis_auth_conflicts_are_secret_free_usage_errors…: 96 subprocess runs
  • unusable_oauth_login_is_distinct_from_missing_credentials: 48 runs
  • service_settings_set_rejects_inexact_numbers…: 30 runs
  • scale invalid values: 13 runs
  • postgres metrics and slow-query date rejections: 10 runs each
  • the scale, tag-conflict, private-endpoint, member-update, key-update and reverse-private-endpoint "…sends no http" tests
• Request bodies the build_*_request tests already assert. About 76 ClickPipe create body-shape tests (~7132–10869) duplicate the builder tests, including the 11 postgres_type_*_serializes tests, one subprocess each. Proposal:
  • Keep cross_source_create_controls_reach_all_eight_request_shapes.
  • Merge the "field absent when omitted" cases into one table test that asserts the exact minimal JSON per source, so the "" serialization bug class is still caught at the wire.
  • Delete the rest. Similar duplicates exist for BYOC, backups, query endpoints, schema discovery and workload identity.
• Behaviour handled centrally, re-tested for each command:
  • About 16 per-command "API error keeps its message or classification" tests all go through convert_error.
  • About 10 per-command "OAuth write rejected" tests all hit the single check at cloud/mod.rs:120.
  • Keep one end-to-end test of each, and replace the rest with a unit table over is_write_command().
  • Keep the Postgres error tests, because postgres.rs uses its own unwrap_api.
• Local subprocess tests that duplicate inline tests:
  • local_version_error_test.rs: the whole file
  • 3 clap-error tests in local_client_selectors_test.rs, one with 32 runs
  • the local_init_json_test human-output tests
  • the docker diagnostics guidance tests
  • local_server_name_compatibility_test conflicting_name_forms_fail_before_dispatch
  • the help and version tests in local_postgres_start_validation_test
  • the telemetry parse-outcome tests that repeat the inline lossy_* tests

F. Too many subprocess runs inside tests worth keeping (~550 runs)

• Loops that re-run whole route matrices for --file and its alias. assert_primary_json_input already unit-tests the alias.
• A diagnosis test that runs 5 states × 3 auth modes × 2 output modes (30 runs), although only the state matters.
• 35 runs over enums × write paths; 18 runs of the generic 403 → exit 4 mapping; 18 runs in api_key_precedence_skips_oauth_refresh_and_preserves_tokens.
• json_and_agent_modes_leave_native_argv_unchanged: 45 runs, although the mode is independent of the input and format.
• Many tests repeat everything in both explicit and agent JSON modes. One test of agent detection is enough.

G. Merge into table tests (no coverage lost)

• dotenv: 24 tests → 3
• version_manager: resolve.rs (~21 fewer), platform.rs (−10), spec.rs (−9), list.rs compare_versions
• discovery.rs
• the backup-config builders, the service_query whoami and credential tests, and the postgres credential, list-filter and config blocks
• telemetry_test.rs (2,805 lines) is about twice the size it needs to be. The exec-handoff, parse-outcome, read-only status and no-leak tests can each become one table test.

Gaps found along the way (worth adding)

• No unit test exists for shared::parse_date_only, api_keys::quote_shell_argument_for_display, upgrade_window_human_view, or the service-settings numeric boundaries. Today only subprocess tests reach them.
• is_write_command() has no per-command table for udf, clickstack or clickpipe.
• There are no shared test helpers. Every tests/local_*.rs re-implements clickhousectl_binary, the fake ClickHouse, unused_port and ProcessGuard.
• Output bug: list_installed_display_empty pins "Run: clickhousectl local install stable" (local/output.rs ~977). Docs and examples are supposed to use latest.

Clearly valuable: keep

• Telemetry: privacy and wire-field tests, end-to-end consent and DO_NOT_TRACK, failure classification.
• Error contracts: the JSON error envelope, exit-code routing, secret redaction.
• Auth: credential and auth precedence, token migration, refusal to follow OAuth redirects.
• Local servers: postgres readiness and rollback against the fake Docker socket, the watchdog and SIGKILL tests, the remove global-guard and lock-race tests.
• Install: archive safety (path traversal, links, gzip CRC) and atomic install races.
• Service query: repair and rollback flows, first-use binding, the wake header, the closed-stdout and closed-stderr regressions.
• Parsing: custom value parsers and conflicts_with/requires rules.
• Builders with real logic: key merge, access-rule planning, merge_tags, BYOC profiles.
• The UDF reload rejection-blame logic.
• The whole-tree structural help tests in src/cli.rs.

Suggested order

1. Add the rule above to AGENTS.md and the add-cli-command skill.
2. Settle the both-copies cases, then delete and merge one area per PR: cli_request_shape_test.rs (the biggest), then the cloud inline tests, then the local inline tests, then the local subprocess and telemetry tests.

🤖 Generated with Claude Code