-
Notifications
You must be signed in to change notification settings - Fork 20k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core: stop chain when tests are finished #27660
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't really necessary. The goroutines will be cleaned up by the runtime when the test exits I see that goroutines will sit around until all tests exit, so that could slow other tests down a bit -- journaling isn't relevant because the tests use in-memory databases (and don't reopen them).
Do you know how much faster the core/blockchain
tests run with this change?
I ran the tests before and after the change, not a notable difference in execution time. The reason for the PR is as you note, the goroutines hang out while other tests are running and it seems better to clean-up resources in tests when not too difficult. Also most other tests call blockchain.Stop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, appears it is more of an omission to not stop the chain. Thanks for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
georgezhang714
core (blockchain_test): add chain.Stop() to tests
This reverts commit 625e847.
This reverts commit 625e847.
Adds calls for chain.Stop() to tests that call NewBlockChain