test: delete 94 tests that assert string literals against themselves - #46
Merged
Conversation
Every one of these had the same shape, one Good/Bad/Ugly triplet per exported
symbol:
func TestRegister_Register_Good(t *core.T) {
ax7Variant := "Register:good"
core.AssertContains(t, ax7Variant, "good")
label := "Register:good"
core.AssertContains(t, label, "Register")
core.AssertContains(t, label, "good")
}
Register is never called. The test asserts that a string literal contains its
own substring, which is true before the package is written and stays true after
it is deleted. Thirteen files were nothing but this; two more carried it
alongside real tests.
The justification is measured, not asserted. Coverage was captured per package
before and after, and every one is byte-identical — same total, same per-function
numbers:
p2p 89.2%, browser 78.2%, chat 61.5%, contextmenu 58.3%, dock 53.1%,
events 76.5%, coreutil 100.0%, textutil 100.0%, keybinding 56.0%,
lifecycle 60.5%, menu 69.3%, systray 63.2%, mcp 55.7%, webkit 28.6%,
php 0.0%
Nothing moved because nothing was covered. That is the whole case for removing
them: they were coverage-shaped noise sitting next to the real suites — p2p's
service_test.go was 27 fakes beside a service_behaviour_test.go that genuinely
opens a Service, publishes, subscribes and asserts on the driver.
WORTH SAYING PLAINLY: engine/php now has no test file. It also had no coverage
before — 0.0% with twelve tests present — so this does not lose anything, it
stops the package advertising a suite it never had. It is the most valuable gap
in the repo now that it is visible.
The banked analyser only found 33 of these, and 17 of those were its own false
positives on scenario-style names. It misses the pattern precisely because the
tautology spells the symbol INSIDE a string literal, so the body looks like it
references the symbol. The criterion used here instead: strip string literals,
then flag any test whose remaining body calls nothing but core.Assert*/Require*.
cd go && GOWORK=off go build ./... exit 0
GOWORK=off go vet ./... exit 0
GOWORK=off go test ./... exit 0, 58 packages ok, 0 failures
gofmt -l 0 offenders
Four more shells remain in display/webkit/pkg/window (register_test.go's three,
window_test.go's one). Left alone deliberately — that package has an in-flight
PR against it and is not mine to touch yet.
Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Snider
added a commit
that referenced
this pull request
Aug 8, 2026
) The four shells #46 could not touch while PR #44 was in flight, now adjudicated per case rather than swept. register_test.go — genuine fake. All three tests asserted that the string "Register:good" contains "good" and never named Register at all. Replaced with a real Good/Bad/Ugly triplet that calls it: the returned constructor yields an OK Result carrying a *Service wired to the given platform with manager and spec registry initialised (Good); a nil platform still constructs rather than panicking during Core wiring, long before any window opens (Bad); and the constructor is reusable, each call producing an INDEPENDENT service — a shared specs map would let two Cores overwrite each other's window registrations (Ugly). window_test.go — mislabelled but real. These tests DO call WithName / WithTitle / WithURL and assert properly; the ax7Variant lines were dead weight prepended to working bodies. Removed those two lines per test, 116 in total, leaving every assertion byte-identical. Receipts: coverage 68.3% before, 68.3% after — nothing real was removed Register 100.0% (was already covered by service_screen_test.go's real use; the deleted shells contributed nothing) go test -race -count=2 clean vet + gofmt clean
Snider
added a commit
that referenced
this pull request
Aug 8, 2026
#51) * test(engine/php): real coverage for detect, env, extract and workspace engine/php had 363 running tests and 0.7% coverage. They were empty bodies and tautologies — `subject := IsLaravelProject; AssertNotNil(t, subject); AssertEqual(t, "good", "good")` takes a func reference (non-nil by construction) and asserts a literal equals itself. Same family PR #46 deleted 94 of elsewhere. Replaced, not deleted, for four files. Coverage 0.7% -> 7.6%. Per function in the files touched: detect.go all 12 at 100.0% probe.go 100.0% / 85.7% workspace.go 100.0% / 93.8% / 84.6% / 100.0% env.go 76.2% / 100.0% / 85.7% / 92.3%, resolveDataDir 35.3% extract.go 70.6% resolveDataDir cannot exceed ~35% in one run: only one GOOS branch executes. The rest of extract.go is core.MkdirTemp / PathRel / MkdirAll failures with no injection point. Tests are hermetic — t.TempDir fixtures, fstest.MapFS for the embedded tree, t.Setenv to redirect HOME so PrepareRuntimeEnvironment never touches a real user profile, t.Chdir for the workspace search. Fault injection is real: truncated and non-JSON composer.json, unreadable octane config, unwritable data dir and Laravel root, a filesystem whose reads fail mid-walk, malformed and unsupported-version workspace.yaml. ONE PRODUCTION CHANGE, from a defect the tests exposed — a second instance of the go-io os.Root class fixed in #49, this time in detect.go. Project probing ran through the coreio local medium, which refuses any path leaving its root, including one that merely traverses a symlink. Handed a symlinked project path — ~/Sites/app pointing at a volume, a git worktree, a bind-mounted container path — every file read as absent, so IsLaravelProject returned false FOR A LARAVEL PROJECT and GetLaravelAppName returned "". Silently: no error, just a wrong answer. probe.go adds probeExists / probeRead over core.Stat / core.ReadFile, the same os-level probing already used by workspace.go and display/webkit/pkg/container. go-io is correct and untouched; coreio media stay for sandboxed I/O. Receipt: probed before the fix — IsLaravelProject(real)=true IsLaravelProject(symlink)=false GetLaravelAppName(real)="Probe" GetLaravelAppName(symlink)="" TestIsLaravelProject_ThroughSymlink now pins both as equal. The medium seam is unaffected in practice: SetMedium has no callers outside its own build-tagged example. Gates: engine/php green under -race; whole library 59 packages ok, 0 failures; vet + gofmt clean. Co-Authored-By: Virgil <virgil@lethean.io> * test(engine/php): scope the Extract leak check to its own temp prefix TestExtract_Bad and TestExtract_Ugly snapshotted the whole system temp directory either side of the call, so any unrelated process creating a temporary file between the two reads would be reported as a leaked extraction. On a busy CI runner that is a flake that looks like a real failure. Only entries carrying Extract's own "go-php-laravel-" prefix are counted now, which is still a true leak check and immune to unrelated activity. Receipt: go test -run TestExtract_ -count=2 ./engine/php/ green. Co-Authored-By: Virgil <virgil@lethean.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every one of these had the same shape — one Good/Bad/Ugly triplet per exported symbol:
Registeris never called. The test asserts that a string literal contains its own substring — true before the package is written, and still true after it is deleted. Thirteen files were nothing but this; two more carried it alongside real tests.The justification is measured, not asserted
Coverage was captured per package before and after. Every package is byte-identical — same total, same per-function numbers:
Nothing moved because nothing was covered. That is the whole case: they were coverage-shaped noise sitting beside the real suites. p2p's
service_test.gowas 27 fakes next to aservice_behaviour_test.gothat genuinely opens a Service, publishes, subscribes and asserts on the driver.Worth saying plainly
engine/phpnow has no test file. It also had no coverage before — 0.0% with twelve tests present — so nothing is lost; the package just stops advertising a suite it never had. It is now the most valuable gap in the repo, and visible.On the analyser
The banked
unreferenced-symbols.pyfound only 33 of these, and 17 of those were its own false positives on scenario-style names. It misses the pattern precisely because the tautology spells the symbol inside a string literal, so the body looks like it references the symbol.The criterion used here instead: strip string literals, then flag any test whose remaining body calls nothing but
core.Assert*/core.Require*.Receipts
Deliberately not included
Four shells remain in
display/webkit/pkg/window(register_test.go's three,window_test.go's one). That package has an in-flight PR against it, so it is not mine to touch yet.🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io