REV: connection path integration tests (#709)

Overview

Good addition. The test file is readable, the env var indirection pattern is sound, and the coverage of groups A through H follows the #709 matrix closely. There are no blocking correctness bugs, but there are a few issues worth addressing before merging.

Issue 1 — CI step missing --release (build waste)

The new CI step runs:

cargo test --test connection_paths

Without --release, Cargo will build a fresh debug binary. The connection-tests job already runs cargo build --release in step 4, so a debug build is redundant. CARGO_BIN_EXE_rpg is profile-aware — with --release it points to the already-built target/release/rpg; without it, Cargo compiles a second binary from scratch.

Fix:

run: cargo test --release --test connection_paths

Issue 2 — A2 and E2 are identical tests

a2_tcp_pgpassword and e2_pgpassword have identical setup: same host/port/user/database, same env, same assertion. The only difference is the doc comment framing. One of them should be removed, or E2 should test something E2-specific (e.g., no CLI connection flags at all — just env vars, including PGHOST/PGPORT pointing at the SCRAM instance).

Issue 3 — G1 does not test trust auth in CI

G1 is documented as Trust auth — connects without any password and uses -w to refuse prompting. But in CI, trust_password() resolves to testpass (from TEST_PGPASSWORD) and the test sets PGPASSWORD=testpass on the subprocess. The target server at port 15432 is testuser:testpass — password auth, not trust auth.

So G1 tests password auth with PGPASSWORD in CI, identical to A1. It will never catch a regression in actual trust auth handling. There is no trust-only postgres in the CI job (no pg_hba.conf entry with trust method), so this is partly a CI topology limitation. Either:

• Accept the limitation and update the doc comment to reflect what CI actually exercises, or
• Add a trust-auth postgres service to the job (e.g., an instance with POSTGRES_HOST_AUTH_METHOD=trust and no password).

Issue 4 — Silent pass for socket-absent tests

B1, B2, and C4 return early with an eprintln! when the socket file is absent:

eprintln!("SKIP b1_socket_explicit_host: {socket_path} not found");
return;

In Rust's standard test harness, return from a test function is a pass. There is no built-in skip. The eprintln! goes to captured stderr that is only shown on failure. The net result: on any CI runner without a local postgres socket, B1/B2/C4 silently report as passed with zero coverage. This is not a correctness bug, but it is an observability problem — you cannot tell from cargo test output whether B1 ran or was skipped.

The ignore attribute with a doc comment is the idiomatic workaround for environment-dependent tests in Rust's stock harness. The test-case or rstest crates also expose skip. If neither is acceptable, at least add an annotation in the test summary comment so the intent is visible.

Issue 5 — F3 assertion is too broad

f3_unknown_host_clear_error accepts any of: "name", "resolve", "lookup", "address", or "host". The "host" keyword in particular will match almost any connection error message — rpg typically echoes the target host in the error context. This assertion would pass even if rpg emitted "error: host doesnotexist.invalid is unreachable" with no DNS-specific wording, which is not useful quality coverage.

Tighten to require at least one of "name or service not known", "failed to resolve", "no such host", or "name resolution" — strings that actually appear in DNS resolution failures across Linux/macOS libc and tokio-dns.

Issue 6 — stdout.contains('1') is ambiguous

A1, B1, C1, C2, C5, E1, H1 all check stdout.contains('1') or stdout.contains('2'). These match any 1 or 2 in the output, including row counts ((1 row)), port numbers echoed back, or format characters. This is unlikely to produce a false positive in practice, but it is also unlikely to catch a regression where the query runs but returns the wrong value.

A more precise check would be stdout.contains("| 1 |")" or asserting the query with an alias: SELECT 1 AS resultand checkingstdout.contains("result")andstdout.contains(" 1 ")`. Not a blocker, but worth tightening.

CI port mapping — confirmed correct

The question raised in the task about CI port mapping:

• New step sets TEST_PG_TLS_PORT=5433. The tls_port() helper reads TEST_PG_TLS_PORT, so TLS tests hit the pg-tls docker container at port 5433. Correct.
• New step sets TEST_PG_SCRAM_PORT=15433. The scram_port() helper reads TEST_PG_SCRAM_PORT, so SCRAM tests hit the postgres-scram service at port 15433. Correct.
• The postgres-scram service uses postgres:16 with only POSTGRES_PASSWORD set. Postgres 14+ defaults pg_hba.conf to scram-sha-256, so this service is genuinely SCRAM auth. Correct.
• The trust port in CI resolves via TEST_PGPORT=15432 from the job-level env, pointing at the main postgres service. Correct.

env var scoping — confirmed correct

cmd.env("PGPASSWORD", ...) scopes to the child process only. It does not mutate the test process environment. No cross-test env var bleed.

CARGO_BIN_EXE_rpg — correct, with note

env!("CARGO_BIN_EXE_rpg") is set by Cargo at compile time to the binary matching the current profile. It works correctly in both cargo test and cargo test --release. No issue here, but see Issue 1 — without --release in the CI command, the env macro points at a debug binary that has to be compiled from scratch.

Missing coverage (noted, not blocking)

Items from #709 not covered by this PR, and not covered by test-connections.sh either:

• PGPASSFILE / .pgpass (covered by the bash compat script but absent from the Rust suite)
• sslmode=disable (covered by bash, absent from Rust suite)
• sslmode=verify-ca / sslmode=verify-full (correctly deferred to fix(ssl): sslmode=verify-ca and verify-full fail with UnknownIssuer when server does not send full chain #712)
• PGCONNECT_TIMEOUT behavior
• Multi-host connection strings (PG 10+ failover syntax)

These are reasonable to defer, but worth a // TODO: add once #712 resolved comment in the D group.

Verdict: REQUEST_CHANGES

Two items require a fix before merge:

1. Add --release to the CI cargo test command (Issue 1 — avoids redundant build).
2. Resolve the A2/E2 duplication — one of them should either be removed or changed to test a distinct scenario (Issue 2).

Issues 3–6 are improvements but not blockers. Issue 3 (G1 semantics) is worth at least a comment fix so the test accurately describes what it covers in CI.

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.80%. Comparing base (dc09357) to head (7da4572).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##             main     #730   +/-   ##
=======================================
  Coverage   68.80%   68.80%           
=======================================
  Files          46       46           
  Lines       31000    31000           
=======================================
  Hits        21327    21327           
  Misses       9673     9673           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.