Five tests check less than their names or comments say #223

New Issue

2026-10-16T01:52:37Z

cgalo5758 commented

2026-10-16 01:52:37 +00:00

What a contributor runs into

A test's name and comment say what it protects. The tests below say more than they check, so the behavior they name can break while they pass. Each entry says what the test checks today.

TestVerifyClaimWorkflowFinalProbeAtDeadline (internal/workflows/domains/verify_test.go:411-450). The comment says the wait before the last probe is clamped to the claim's deadline, so a record published during the last backoff interval is still caught. The test sets the deadline 45 seconds out, stubs two missing TXT probes and then a match, and checks that the workflow ends verified and that the three probes ran. It never reads the clock. Without the clamp (clampedWait, internal/workflows/domains/verify.go:642, called at :490) the third probe runs at 90 seconds instead of 45, still matches, and the test passes; a run with the clamp removed did exactly that. The trace scenario final-probe-at-deadline (internal/workflows/domains/verify_trace_test.go:198) does pin it: its golden, internal/workflows/domains/testdata/verify_trace/final-probe-at-deadline.txt, records a 15-second timer and the third probe at +45s.

TestGoCopyDirsCoverTheCopyPackages (internal/lint/anatomy_test.go:256-270). The comment says it pins the packages the lint's Go copy pass reads: the server, the shared web helpers and each integration's web package. It joins the list goCopyDirs returns (internal/lint/anatomy.go:679-683) into one string and checks that each wanted name is a substring of it. "web" is a substring of "integrations/fedwiki/web", so a goCopyDirs that drops the shared web helpers still passes; a run with that entry removed did. TestLintVerdicts now prints the exact list into its golden (internal/lint/verdicts_test.go:179-182, internal/lint/testdata/verdicts.golden:44-48).

Three readiness subtests in internal/server/operator_product_readiness_render_test.go:

"sync-pending renders the amber pending badge" (:196-202) counts one badge reading "Sync pending" and five reading "Met".
"sync-failed renders the red failed badge and a Retry action" (:227-238) counts one badge reading "Sync failed" and checks the Retry button and the error text.

Neither reads the badge's class, so a pending badge rendered grey or green passes. The tone comes from the badge map (internal/server/anatomy.go:317, :323) through the statusBadge partial, which renders it as text-bg-warning or text-bg-danger (internal/embeds/templates/partials/ui_badge.html:12).
"plan ladders section shows name leading, key muted" (:255-267) checks that the ladder row's heading reads the ladder's name and nothing else. Its own comment says the page shows no key at all, so nothing on the page is muted.

TestOrgTypeDefaultChange_ConcurrentCommitsSerialize (internal/server/operator_org_type_change_test.go:576-627). The comment says the per-pool lock serializes two concurrent commits of one default change, and the test says it holds "no matter how the two runs interleaved". It starts two goroutines and lets the scheduler pick the order. Nothing holds either request until both have read the default or both wait on the pool lock, so a run in which one commit finishes before the other starts passes without the lock ever being contended. TestDefaultChangeTwiceSerializesOnThePool (internal/server/plan_acts_concurrency_db_test.go:253-291) holds both requests until both are queued on the pool lock, then checks both answers and the rows.

TestGrantDeliveryState_EndToEnd (internal/server/operator_grants_delivery_state_test.go:66-154). Its last part says the Grants index and the organization page must agree. It requests the index with no search and no filter (:147-153), which is page 1 of every grant in the shared test database. assertDeliveryRendering (:159-176) then checks only that the page contains a Live badge, a Superseded badge, an Inactive badge and "Replaced by" somewhere. Grants other tests left behind satisfy every check, so this part passes even when none of this test's grants is on the page. The organization is named "Delivery State Org" with no suffix (:71), so each run adds another organization of that name.

Why it costs

A reader trusts a test's name and comment without reading its body. When they claim more than the body checks, a change that breaks the claim passes review and the suite, and the next person takes the protection as given. Nothing in the suite flags these; they were found by breaking the code under test or by reading each assertion against the name.

Where

internal/workflows/domains/verify_test.go
internal/lint/anatomy_test.go
internal/server/operator_product_readiness_render_test.go
internal/server/operator_org_type_change_test.go
internal/server/operator_grants_delivery_state_test.go

Done when

TestVerifyClaimWorkflowFinalProbeAtDeadline asserts that the workflow ends 45 seconds after it starts, as the test above it checks its elapsed time (verify_test.go:404-407), and fails with the clamp removed. Or it is deleted, and the trace scenario holds the clamp alone.
TestGoCopyDirsCoverTheCopyPackages compares the exact list, each entry relative to its temporary directory, and fails when the shared web helpers drop out. Or it is deleted, since TestLintVerdicts pins the same list.
The amber and red subtests select span.badge.text-bg-warning and span.badge.text-bg-danger. The third subtest is renamed to what it checks, for example "plan ladders section shows the ladder's name alone".
TestOrgTypeDefaultChange_ConcurrentCommitsSerialize is deleted, since TestDefaultChangeTwiceSerializesOnThePool covers the contended order. If it stays, it is renamed to what it checks (two commits of one change leave one grandfathered pool), and its comment drops the claim about interleavings.
TestGrantDeliveryState_EndToEnd gives its organization a unique name, requests the index with ?q= set to that name, and checks this test's three grants by grant id. Finishing #192 would also keep other tests' grants off the page, but the filter does not need to wait for it.

## What a contributor runs into A test's name and comment say what it protects. The tests below say more than they check, so the behavior they name can break while they pass. Each entry says what the test checks today. **`TestVerifyClaimWorkflowFinalProbeAtDeadline`** (`internal/workflows/domains/verify_test.go:411-450`). The comment says the wait before the last probe is clamped to the claim's deadline, so a record published during the last backoff interval is still caught. The test sets the deadline 45 seconds out, stubs two missing TXT probes and then a match, and checks that the workflow ends verified and that the three probes ran. It never reads the clock. Without the clamp (`clampedWait`, `internal/workflows/domains/verify.go:642`, called at `:490`) the third probe runs at 90 seconds instead of 45, still matches, and the test passes; a run with the clamp removed did exactly that. The trace scenario `final-probe-at-deadline` (`internal/workflows/domains/verify_trace_test.go:198`) does pin it: its golden, `internal/workflows/domains/testdata/verify_trace/final-probe-at-deadline.txt`, records a 15-second timer and the third probe at +45s. **`TestGoCopyDirsCoverTheCopyPackages`** (`internal/lint/anatomy_test.go:256-270`). The comment says it pins the packages the lint's Go copy pass reads: the server, the shared web helpers and each integration's web package. It joins the list `goCopyDirs` returns (`internal/lint/anatomy.go:679-683`) into one string and checks that each wanted name is a substring of it. "web" is a substring of "integrations/fedwiki/web", so a `goCopyDirs` that drops the shared web helpers still passes; a run with that entry removed did. `TestLintVerdicts` now prints the exact list into its golden (`internal/lint/verdicts_test.go:179-182`, `internal/lint/testdata/verdicts.golden:44-48`). **Three readiness subtests** in `internal/server/operator_product_readiness_render_test.go`: - "sync-pending renders the amber pending badge" (`:196-202`) counts one badge reading "Sync pending" and five reading "Met". - "sync-failed renders the red failed badge and a Retry action" (`:227-238`) counts one badge reading "Sync failed" and checks the Retry button and the error text. Neither reads the badge's class, so a pending badge rendered grey or green passes. The tone comes from the badge map (`internal/server/anatomy.go:317`, `:323`) through the `statusBadge` partial, which renders it as `text-bg-warning` or `text-bg-danger` (`internal/embeds/templates/partials/ui_badge.html:12`). - "plan ladders section shows name leading, key muted" (`:255-267`) checks that the ladder row's heading reads the ladder's name and nothing else. Its own comment says the page shows no key at all, so nothing on the page is muted. **`TestOrgTypeDefaultChange_ConcurrentCommitsSerialize`** (`internal/server/operator_org_type_change_test.go:576-627`). The comment says the per-pool lock serializes two concurrent commits of one default change, and the test says it holds "no matter how the two runs interleaved". It starts two goroutines and lets the scheduler pick the order. Nothing holds either request until both have read the default or both wait on the pool lock, so a run in which one commit finishes before the other starts passes without the lock ever being contended. `TestDefaultChangeTwiceSerializesOnThePool` (`internal/server/plan_acts_concurrency_db_test.go:253-291`) holds both requests until both are queued on the pool lock, then checks both answers and the rows. **`TestGrantDeliveryState_EndToEnd`** (`internal/server/operator_grants_delivery_state_test.go:66-154`). Its last part says the Grants index and the organization page must agree. It requests the index with no search and no filter (`:147-153`), which is page 1 of every grant in the shared test database. `assertDeliveryRendering` (`:159-176`) then checks only that the page contains a Live badge, a Superseded badge, an Inactive badge and "Replaced by" somewhere. Grants other tests left behind satisfy every check, so this part passes even when none of this test's grants is on the page. The organization is named "Delivery State Org" with no suffix (`:71`), so each run adds another organization of that name. ## Why it costs A reader trusts a test's name and comment without reading its body. When they claim more than the body checks, a change that breaks the claim passes review and the suite, and the next person takes the protection as given. Nothing in the suite flags these; they were found by breaking the code under test or by reading each assertion against the name. ## Where - `internal/workflows/domains/verify_test.go` - `internal/lint/anatomy_test.go` - `internal/server/operator_product_readiness_render_test.go` - `internal/server/operator_org_type_change_test.go` - `internal/server/operator_grants_delivery_state_test.go` ## Done when - `TestVerifyClaimWorkflowFinalProbeAtDeadline` asserts that the workflow ends 45 seconds after it starts, as the test above it checks its elapsed time (`verify_test.go:404-407`), and fails with the clamp removed. Or it is deleted, and the trace scenario holds the clamp alone. - `TestGoCopyDirsCoverTheCopyPackages` compares the exact list, each entry relative to its temporary directory, and fails when the shared web helpers drop out. Or it is deleted, since `TestLintVerdicts` pins the same list. - The amber and red subtests select `span.badge.text-bg-warning` and `span.badge.text-bg-danger`. The third subtest is renamed to what it checks, for example "plan ladders section shows the ladder's name alone". - `TestOrgTypeDefaultChange_ConcurrentCommitsSerialize` is deleted, since `TestDefaultChangeTwiceSerializesOnThePool` covers the contended order. If it stays, it is renamed to what it checks (two commits of one change leave one grandfathered pool), and its comment drops the claim about interleavings. - `TestGrantDeliveryState_EndToEnd` gives its organization a unique name, requests the index with `?q=` set to that name, and checks this test's three grants by grant id. Finishing #192 would also keep other tests' grants off the page, but the filter does not need to wait for it.

cgalo5758 added the

labels 2026-10-16 01:52:37 +00:00

Sign in to join this conversation.