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

Codecov Report

❌ Patch coverage is 99.82548% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 69.43%. Comparing base (c1b21d2) to head (7e2054b).

@@            Coverage Diff             @@
##             main     #702      +/-   ##
==========================================
+ Coverage   68.38%   69.43%   +1.05%     
==========================================
  Files          46       46              
  Lines       31265    31838     +573     
==========================================
+ Hits        21379    22104     +725     
+ Misses       9886     9734     -152     

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

REV Code Review

• PR: test: boost unit test coverage from 68% toward 75% #702 - feat/coverage-boost: add unit tests to boost coverage from 68% toward 75%
• Author: @NikolayS
• AI-Assisted: Yes (REV — postgres-ai/rev)

BLOCKING ISSUES (3)

HIGH src/update.rs:146 — Wrong GitHub API URL leaks internal repo name

GITHUB_API_URL points to NikolayS/project-alpha (internal codename) instead of NikolayS/rpg. This means rpg will never find a valid update and may error on update checks. The public repo is NikolayS/rpg.
Fix: Change to https://api.github.com/repos/NikolayS/rpg/releases/latest

HIGH src/repl/mod.rs — SIGWINCH handler removed, terminal resize broken

The PR removes the entire SIGWINCH signal handler that kept the status bar redrawn when the terminal is resized while a query is running. The removal is a side effect of unwrapping Arc<Mutex<StatusLine>> into a plain StatusLine — which is correct for single-threaded access — but no equivalent resize mechanism replaces it. sl.on_resize() is called only before each readline prompt, so resizing during a long-running query will leave the status bar corrupted (wrong width) until the next prompt. The SIGWINCH handler must be restored (e.g., via a channel to the readline loop) or an alternative resize-detection strategy must be added.
Fix: Restore terminal resize handling. The simplest approach is an Arc<AtomicU16> pair for cols/rows updated from a SIGWINCH task, read by on_resize() each render cycle.

HIGH tests/compat/test-connections.sh — test_wrong_password now uses trust-auth host

The test test_wrong_password (section G) was previously guarded by TEST_PG_SCRAM_PORT: it only ran against a SCRAM-auth server where a wrong password is guaranteed to fail. The PR removes that guard and now unconditionally runs the test against TEST_PGHOST:TEST_PGPORT — the default trust-auth server. On a trust-auth server a wrong PGPASSWORD is silently ignored and the connection succeeds with exit 0, so expect_failure will report a test failure in CI. The test tests nothing meaningful on a trust-auth server.
Fix: Either re-add the TEST_PG_SCRAM_PORT guard (and skip if not set), or document that TEST_PGHOST must be SCRAM-auth.

NON-BLOCKING (5)

MEDIUM src/connection.rs — Multi-host URI: last-port-reuse changes libpq semantics

The new parse_uri manually reuses the last explicit port for subsequent hosts without one (h1,h2:5433,h3 → h3 gets 5433). The old code delegated to tokio_postgres::Config which assigns 5432 to hosts without explicit ports — matching real libpq behavior. The test at line 3706 was updated to assert the new "port reused" behavior. This is a deliberate semantic change, but it diverges from libpq/psql. If a user writes postgres://primary,replica/db expecting both to use 5432, they get it. But postgres://primary:5433,replica/db would give replica port 5433, not 5432. This is not what psql does and may surprise users. No tests cover the case where a user actually wants two hosts at different ports (e.g., h1:5432,h2:5433).
Suggestion: Document the divergence from libpq explicitly in a code comment, and add a test for h1:5432,h2:5433,h3:5434 (fully explicit) vs h1:5432,h2:5433,h3 (last-port-reuse).

MEDIUM src/connection.rs — default_host() loses port information

default_host_port() was replaced with default_host() which always returns port 5432. The old implementation scanned socket directories for any .s.PGSQL.<port> and returned the actual port found. Any Postgres instance listening on a non-5432 Unix socket will no longer be auto-detected and connected to; users connecting without explicit -p will get a misleading "connection refused" on port 5432 instead of connecting automatically. This regresses issue #728 which was explicitly listed in the CHANGELOG (now removed).
Suggestion: This appears to be an intentional rollback; add a comment explaining why and file a follow-up issue if the behavior is to be restored.

MEDIUM src/connection.rs — SSL/TLS capability regression: sslmode=require no longer uses NoVerifier

make_tls_config_require (which accepted self-signed certs without verification) has been removed. sslmode=require now calls connect_tls_default which uses make_tls_config_default (the standard rustls verifier with no client cert support). This means sslmode=require will now reject self-signed certificates — exactly the regression that was fixed in v0.8.1 (#711). The CI TLS tests (Section D) and the entire tests/connection_paths.rs integration test suite have also been deleted, removing the regression guard.
Fix: Restore make_tls_config_require with the NoVerifier, or ensure connect_tls_default implements no-verify semantics for sslmode=require.

LOW src/explain/share.rs:388 — extract_depesz_url test: the "no /s/ path" case may be over-specified

extract_depesz_url_returns_none_when_only_domain_without_s_path uses https://explain.depesz.com/about. The function only matches lines containing explain.depesz.com/s/, so any URL without /s/ will return None. The test is correct but the comment "A URL without the /s/ path should not match" is slightly misleading — /s/ is the entire key predicate. Minor; no action required.

LOW src/repl/mod.rs — Removed ENV_MUTEX on parallel env-var tests

The ENV_MUTEX serialization guard was removed from four tests that call std::env::set_var / remove_var. Rust's test harness runs unit tests in parallel by default. These tests are now racy: two tests modifying RPG_TEST_API_KEY_12345 or PSQLRC concurrently can interfere. The tests may pass individually but flake under parallel execution.
Suggestion: Either restore a per-module LazyLock<Mutex<()>> guard or use std::sync::OnceLock-based serialization. Alternatively, use unique env var names per test to avoid cross-contamination.

POTENTIAL ISSUES (2)

MEDIUM src/statusline.rs — format_status_truncated_when_content_exceeds_terminal_width may be fragile (confidence: 6/10)

The test sets term_cols = 10 with a 49-char conn_label. However format_status assembles conn_label + mode + tx + ... before truncating. With term_cols=10 the outer format becomes " very-lon │ SQL │ tx:idle " before truncation. Whether the resulting 10-char string is well-formed depends on the exact format assembly. If the inner logic changes (e.g., a new status segment) this test will silently pass because it only checks chars().count() == 10, not content correctness. Consider also asserting non-empty content (no panic/empty).

LOW src/main.rs — apply_cli_pset_border_clamped_to_2 tests implementation detail (confidence: 5/10)

The test asserts border=5 clamps to 2. This is correct behavior, but the test name and assertion test the specific clamping constant rather than the user-visible behavior (e.g., "border values above 2 are treated as 2"). Minor nit; acceptable as-is.

Summary

Note:

• Findings: High-confidence issues (8-10/10) — blocking or non-blocking per severity
• Potential: Medium-confidence issues (4-7/10) — review manually
• Filtered: Low-confidence issues (0-3/10) — excluded as likely false positives

Test quality assessment

The new unit tests add ~730 lines of coverage across five modules. Here is a frank assessment of quality:

What's good:

• large_object.rs tests (hex_encode, hex_digit, decode_bytea_hex) are solid: boundary conditions (empty, all-zeros, 0xff, full 256-byte roundtrip), error cases (odd length, invalid digits, wrong prefix), and uppercase/mixed-case acceptance. These test behavior, not implementation.
• statusline.rs tests cover the threshold boundary at 1000 ms (999 ms → ms, 1000 ms → seconds), which is a genuine edge case worth having.
• explain/share.rs tests for extract_depesz_url cover the major cases (href attribute, bare URL, single/double quote delimiters, whitespace, no match).

What's coverage theater:

• main.rs apply_cli_pset tests: 36 tests for a simple match/assign function. Each test has a single assert_eq!. The tests verify individual branches but there is no test for interaction between multiple pset calls (e.g., does setting format=csv then tuples_only=on preserve both?). The tests add line coverage but little behavioral confidence.
• version_string_contains_built_keyword asserts v.contains("built ") — this is testing a hardcoded string literal in the implementation, not user-visible behavior. If the format changes, both the code and the test change together with zero detection value.
• apply_cli_pset_unknown_option_is_silently_ignored only checks that pset.format is unchanged. It does not verify that no panic occurs or that other fields remain unchanged. An unknown option silently mutating border would pass this test.

Missing edge cases for new code paths:

1. parse_uri (new custom URI parser) — the hand-rolled parser replacing tokio_postgres::Config::from_str() has no unit tests. The diff adds 150+ lines of new parsing logic with no test coverage for: percent-encoded usernames/passwords, passwords containing @ or :, empty username (postgresql://:pass@host/db), URI with no database component (postgresql://host), IPv6 addresses without brackets (unusual but accepted by the code), connect_timeout query param parsing, unknown query params ignored correctly.

2. classify_connect_error tests deleted — 6 unit tests for SSL error classification were removed along with the functions they tested. The map_connect_error rewrite has no unit tests. Error message classification is critical for user-visible output quality; the regression risk is high.

3. default_host() rollback — default_host_port() was the most complex logic replaced. No tests exist for default_host() (trivially simple, but the behavioral change is meaningful).

4. strip_psql_table_format tests — the test strip_psql_table_format_handles_pipe_delimited_content may be testing a format that psql never actually produces for EXPLAIN output. psql's \pset border 2 produces |-bordered tables, but EXPLAIN output in aligned format uses space-padding, not pipes. The test may be covering dead code paths.

Verdict: CHANGES REQUESTED. The three blocking issues must be addressed before merge:

1. Fix GITHUB_API_URL (project-alpha → rpg)
2. Restore terminal resize handling (SIGWINCH or equivalent)
3. Fix or guard test_wrong_password to use SCRAM auth

The SSL regression (sslmode=require with self-signed certs) and the env-var test raciness are important follow-ups if not addressed in this PR.

REV-assisted review (AI analysis by postgres-ai/rev)

REV Review — PR #702: test: boost unit test coverage

Reviewed against current main (c1b21d2, v0.8.3). Previous automated review posted incorrect findings based on a stale diff — disregard it entirely.

What this PR does

Pure test additions across 6 files. 108 new test functions, zero production code changes:

• src/explain/share.rs — extract_depesz_url, hex_encode, hex_digit, decode_bytea_hex
• src/large_object.rs — large object command parsing
• src/lua_commands.rs — Lua command stubs
• src/main.rs — CLI flag parsing helpers
• src/repl/execute.rs — statement execution helpers
• src/statusline.rs — status bar rendering

PRAISE — Thorough coverage of URL extraction edge cases

extract_depesz_url tests cover href attribute, bare URL, single/double quotes, whitespace delimiters, empty body, no match — all realistic inputs a scraping function would encounter. Good.

PRAISE — Hex codec tests are exhaustive

Every boundary (zero byte, FF byte, all-numeric, all-alpha, uppercase, lowercase, zero-padding) covered. Clean parametric style.

MINOR — share_unknown_service_case_sensitive tests accidental behavior

fn share_unknown_service_case_sensitive() {
    // "Depesz" (capital D) → unknown service error

share_plan presumably dispatches on lowercase strings. Testing that "Depesz" fails is fine, but there's no corresponding test that "depesz" succeeds — so we're only testing the error path, not the success path of the actual dispatch. Add a positive case or rename the test to share_unknown_service_returns_error_for_nonexistent.

MINOR — statusline tests: no test for multi-width Unicode truncation

StatusLine::render() truncates at terminal width. The tests cover ASCII content but not multi-byte/wide characters (CJK, emoji). Not blocking for a coverage-boost PR, but worth a follow-up issue.

NIT — tokio::runtime::Builder in test

src/main.rs tests build a Tokio runtime with enable_all(). That's heavyweight for a unit test — current_thread() is sufficient unless the code under test actually spawns tasks. Non-blocking.

Testing evidence

CI green (all 16 checks pass). No production code changed — no regression risk. The 108 new tests will be reported by cargo test on any future diff touching these modules.

Overall verdict: APPROVE

No blocking issues. Pure coverage improvement with solid test quality.