integration/rpctest: improve rpctest harness#2571
Conversation
Initialize the Harness API with a options struct instead of several fields that may be set to the zero value of the type.
Now, Harness TearDown procedure returns an error if the btcd process stopped with error status code. Also, SkipCleanup option was added to TearDown, so now we can write tests that restart the node while keeping the state.
The package debugstream provides two utility types, the Stream and the Client. The Stream type listens in a TCP endpoint and can be used to broadcast events to all connected clients. The event type has a uint64 code and a byte slice data fields. The Stream implementation is used only when btcd is compiled with the debug tag, otherwise a nop implementation is used. The Client connects with the Stream via TCP and starts receiving the events, which are passed into a handler callback, allowing assertions inside tests. All events broadcasted in the stream are stored in memory and sent to all clients after connection, this way if the client connects after the broadcast of events, it still receives all of them.
btcd: integrated debugstream, now we can send debug events and improve testing capabilities. The hidden flag --debugstream=<host:port> starts the debug Stream when btcd is compiled with the debug tag. integration: added debugstream integration test. integration/rpctest: added DebugStreamHandler to harness options, allowing integration tests to easily process debug events. The btcd executable build to be used by the Harness is now being built with the debug tag, but the debug stream will be started only if the DebugStreamHandler is also passed to the Harness.
Previously the [Harness.SetUp] procedure was taking a boolean to generate a test chain and a integer for the required number of mature outputs, but internally, the test chain was being generated only if the number of mature outputs was different than zero and the boolean was true, this means that we can remove the boolean and pass zero when we don't want to create the test chain. After the change, all the tests still passing.
allocz
force-pushed
the
improve_rpctest_harness
branch
from
14d655f to
e5dfa5d
Compare
Fixed a goroutine leak caused by the [memWallet.Start()] procedure which was spinning up a goroutine that was never terminated. Also, added a new method [memWallet.Stop()], which is called by [Harness.TearDown()]. Added [Harness.DisableWalletSyncWait], to avoid blocking when [Harness.SetUp()] is called after shutting down the node before all blocks being processed by the wallet.
| func New(activeNet *chaincfg.Params, handlers *rpcclient.NotificationHandlers, | ||
| extraArgs []string, customExePath string) (*Harness, error) { | ||
|
|
||
| func New(opts ...HarnessOpts) (*Harness, error) { |
There was a problem hiding this comment.
The ...HarnessOpts parameter seems a little misleading since only the first element is ever used. Would it make sense to accept a *HarnessOpts instead, with nil representing the default configuration? That would make it clearer that only a single optional configuration is expected and avoid callers accidentally passing multiple option structs that are silently ignored.
| @@ -242,13 +262,24 @@ func (n *node) cleanup() error { | |||
| // shutdown terminates the running btcd process, and cleans up all | |||
There was a problem hiding this comment.
Here in shutdown, the else if err != nil { return } path skips cleanup() entirely when stop() returns a non-exit error.
This means the pid file and temp directory are left behind if the process fails unexpectedly.
Worth documenting whether this is intentional—callers who need to restart after an unexpected failure would need to handle cleanup themselves.
| ) | ||
| for i := range maxConnAttempts { | ||
| if i > 0 { | ||
| time.Sleep((time.Millisecond*100)<<i - 1) |
There was a problem hiding this comment.
I think the backoff calculation here may not be doing what's intended. Since << has higher precedence than -, the expression is evaluated as ((time.Millisecond * 100) << i) - 1. That means when i == 1, the delay becomes 200ms - 1ns rather than 100ms. Would wrapping it as (time.Millisecond * 100) << (i - 1) better match the intended exponential backoff?
2 participants
Change Description
The main motivation of this PR is to improve the process of writing integration
tests, currently there's no way to write complex tests in a simple way.
As discussed in #2560.
Changes
without cleaning up node data, enabling the test of scenarios where btcd
exits with error or scenarios where the node should be restarted.
called.
multiple zero values to get default initialization.
The debug event stream is replaced by a NOP implementation when the compile tag
debug is not defined, so that the compiler can optimize out all the debug event
broadcast calls. Also, the hidden flag --debugstream=host:port should be used
to start the event stream.
Steps to Test
Pull Request Checklist
Testing
Code Style and Documentation
📝 Please see our Contribution Guidelines for further guidance.