fix(teststate): stop after reporting a fatal error - #1880
Conversation
Every t.Fatalf in this package fell through to the code after it. With *testing.T that is invisible, since FailNow calls runtime.Goexit, but TestingT exists so other harnesses can be plugged in and an implementation whose FailNow returns kept going. The harmful case was save: a marshal failure was reported and then os.WriteFile ran anyway with a nil byte slice, leaving a zero byte file for a later stage to load. IsPresent and IsEmptyJSON were quieter but wrong in the same way, reporting absent or empty when the real answer was an error they had just announced. Add an explicit return after each Fatalf, with a package comment explaining why they are not dead code.
WalkthroughThe change adds explicit returns after ChangesTeststate error handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/core/teststate/teststate.go (1)
124-136: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve error state across boolean helpers.
When
FailNowreturns,IsEmptyJSONreports invalid JSON and returnsfalse.IsPresenttreats that value as “not empty” and returnstrue. An invalid file can therefore be marked present.The same ambiguity makes
savetreat anIsPresentread failure as “absent” and continue towardos.WriteFile. Use an internal error-returning helper or another explicit error signal. Add regression tests throughIsPresentandSave.Also applies to: 157-158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/core/teststate/teststate.go` around lines 124 - 136, Preserve read/JSON-parse errors from the shared test-data validation used by IsEmptyJSON and IsPresent instead of converting them to an ambiguous false result after t.Fatalf. Update save to stop and propagate the same error when presence checking fails, rather than treating the file as absent and calling os.WriteFile; add regression coverage for both IsPresent and Save invalid-file paths.
🧹 Nitpick comments (2)
modules/core/teststate/teststate.go (1)
10-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the package comment accurate.
The comment says every
t.Fatalfhas an explicitreturn. Theos.WriteFilefailure insaveat Line [97] and the JSON unmarshal failure inLoadat Line [115] do not. Add those returns, or narrow the comment to failure branches followed by more work.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/core/teststate/teststate.go` around lines 10 - 12, Update the failure branches in save and Load so the os.WriteFile error and JSON unmarshal error each return immediately after t.Fatalf, keeping the package comment accurate and preventing execution from continuing in harnesses whose FailNow returns.modules/core/teststate/teststate_test.go (1)
247-264: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for
Load's read-failure guard.The new reporter and tests cover
Save,IsPresent, andIsEmptyJSON, but notLoad. Before this return, a non-stopping reporter could letLoadcontinue intojson.Unmarshaland record a second failure. Add a test that uses an unreadable path and asserts that only the read failure is recorded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/core/teststate/teststate_test.go` around lines 247 - 264, Add a test covering teststate.Load with an unreadable path, using the existing non-stopping reporter pattern. Assert that the read failure is reported and only one failure is recorded, confirming Load returns before attempting json.Unmarshal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@modules/core/teststate/teststate.go`:
- Around line 124-136: Preserve read/JSON-parse errors from the shared test-data
validation used by IsEmptyJSON and IsPresent instead of converting them to an
ambiguous false result after t.Fatalf. Update save to stop and propagate the
same error when presence checking fails, rather than treating the file as absent
and calling os.WriteFile; add regression coverage for both IsPresent and Save
invalid-file paths.
---
Nitpick comments:
In `@modules/core/teststate/teststate_test.go`:
- Around line 247-264: Add a test covering teststate.Load with an unreadable
path, using the existing non-stopping reporter pattern. Assert that the read
failure is reported and only one failure is recorded, confirming Load returns
before attempting json.Unmarshal.
In `@modules/core/teststate/teststate.go`:
- Around line 10-12: Update the failure branches in save and Load so the
os.WriteFile error and JSON unmarshal error each return immediately after
t.Fatalf, keeping the package comment accurate and preventing execution from
continuing in harnesses whose FailNow returns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fa357bfb-a016-4ab9-b687-a40eecdc9128
📒 Files selected for processing (2)
modules/core/teststate/teststate.gomodules/core/teststate/teststate_test.go
denis256
left a comment
denis256
left a comment
There was a problem hiding this comment.
Hm, the package comment says every Fatalf has a return, but three calls at the end of functions are missing them:
- Line 97 in teststate.go: save() (on os.WriteFile failure)
- Line 115 in teststate.go: Load() (on json.Unmarshal failure)
- Line 199 in teststate.go: Cleanup() (on os.Remove failure)
2 participants
Found while reviewing #1879. Pre-existing.
Bug
Every
t.Fatalfincore/teststatefalls through to the code after it. With*testing.Tthat is invisible, becauseFailNowcallsruntime.Goexit. ButTestingTexists so other harnesses can be plugged in, and an implementation whoseFailNowreturns keeps going.The harmful case is
save:A later stage then loads that empty file.
IsPresentandIsEmptyJSONwere quieter but wrong the same way, answering "absent" or "empty" when the real answer was an error they had just announced.Fix
An explicit
returnafter eachFatalf, plus a package comment explaining why they are not dead code.Tests
Three regression tests driven by a
TestingTwhoseFailNowreturns. The marshal one was confirmed to fail without the fix.Summary by CodeRabbit
Bug Fixes
Tests