Automatic TearDown on Setup errors #18
Merged
Conversation
This file contains 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
matheusd
force-pushed
the
autoteardown
branch
from
79207a2
to
9ac3a93
Compare
davecgh
reviewed
Oct 18, 2023
|
|
||
| // There should be only 2 goroutines live. | ||
| prof := pprof.Lookup("goroutine") | ||
| gotCount, wantCount := prof.Count(), 2 |
There was a problem hiding this comment.
This is a technique I've not seen before. I typically do this with channels and/or waitgroups.
I don't have any big objection to it, but this is probably fairly fragile since I suspect it relies on the semantics of the testing framework creating 2 goroutines.
Maybe it should get the count prior to calling SetUp and then ensuring it matches that value here so it's robust against framework changes?
There was a problem hiding this comment.
Point taken. Updating to use before/after count of goroutines. The "2" is indeed due to how the test runtime spawns the individual test functions.
This adds an automatic TearDown() call for an rpc harness that fails to be setup, so that callers do not need to remember to do it themselves. This improves usability of the package by ensuring goroutines do not leak after either a failed SetUp() or a TearDown(). Unit tests are added that assert the correct behavior on both a successfull setup and when certain specific setup calls fail. This also fixes a goroutine leak in the internal memory wallet.
This adds a flag to the Harness to instruct it to keep the node dir after TearDown and, unless that flag is set, removes the node dir. This improves the reliability of the test harness by not leaving behind extraneous, unneeded data by default. When calling the TearDownInTest variant, if the test has already failed then the node dir is kept to ease debugging test failures.
matheusd
force-pushed
the
autoteardown
branch
from
9ac3a93
to
5399d59
Compare
davecgh
approved these changes
Oct 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fix #9
This makes failures to run a harness'
SetUp()function to automatically callTearDown()to remove any leaking goroutines.Additionally, it fixes a shutdown goroutine leak in the memory wallet and adds a flag to force the harness to keep the node data intact on TearDown.