testutils: don't deadlock on Recorder overflow; restore cwd via t.Cleanup - #30
Open
evgeny-boger wants to merge 2 commits into
Open
testutils: don't deadlock on Recorder overflow; restore cwd via t.Cleanup#30evgeny-boger wants to merge 2 commits into
evgeny-boger wants to merge 2 commits into
Conversation
…t testing 1. Recorder.Rec deadlocked the system under test when the 1000-item record buffer filled: the producer (typically the driver goroutine, mid-transaction) blocked forever, wedging any test that generates more MQTT records than it Verifies. Seen with a rule script defining a virtual device with dozens of controls. Rec now drops on overflow with a loud 'REC OVERFLOW (dropped)' log marker - a later Verify of a dropped item fails visibly with 'timeout' instead of hanging. 2. SetupTempDir chdirs into the temp dir and only restored the working directory via the returned cleanup func. A test failing hard (FailNow, panic) skips explicit teardown, stranding the whole test process chdir'd into a removed directory - silently breaking any cwd-relative logic (os.Getwd + relative paths) in every subsequent test, far from the original failure. The restore now also runs via t.Cleanup, which fires on all exit paths; the returned func remains for explicit calls and both orders are safe. Both fixes are covered by testutils/robustness_test.go. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AoTGMFABCCENSPaHThxAru
From-scratch PR review findings: - Rec now no-ops once the owning test ends (t.Cleanup flag): producers like driver goroutines can outlive the test, and a post-test t.Log panics the whole binary - the drop path had widened that window. - Document the drop-newest policy and its SkipTill implication. - Pin the explicit-cleanup-then-auto-cleanup order with a subtest; use strconv.Itoa in the test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AoTGMFABCCENSPaHThxAru
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.
Two
testutilsbugs surfaced while running 663 wild wb-rules scripts through the engine test harness (wirenboard/wb-rules#221). Both turn one test's failure into confusing failures elsewhere:Recorder.Recdeadlocks the system under test when the buffer fills. The 1000-item channel send blocks the producer — typically the driver goroutine mid-transaction — so any test generating more MQTT records than itVerifys hangs forever (reproduced with a rule script defining a device with dozens of controls:defineVirtualDevicenever returned). Now overflow drops with aREC OVERFLOW (dropped)log marker; a laterVerifyof a dropped item fails visibly withtimeoutright after the marker instead of hanging the run.SetupTempDirstrands the process cwd on hard test failure. It chdirs into the temp dir and restores only via the returned cleanup func — whichFailNow/panic paths skip. The whole test process then keeps running chdir'd into a removed directory, silently breakingos.Getwd+relative-path logic (e.g. wb-rules'../modulesresolution) in unrelated later tests. The restore now also runs viat.Cleanup, which fires on all exit paths; the returned func remains for explicit calls, and running both is safe.Covered by the new
testutils/robustness_test.go.🤖 Generated with Claude Code
https://claude.ai/code/session_01AoTGMFABCCENSPaHThxAru