[tests] nits in dbcrash.py #10704
[tests] nits in dbcrash.py #10704
Conversation
|
Looks like this test is acting up: |
sdaftuar
left a comment
There was a problem hiding this comment.
Looks good, thanks for cleaning this up. Please take a look at the fixes needed to make travis happy as well.
test/functional/dbcrash.py
Outdated
There was a problem hiding this comment.
I don't feel strongly, but the reason I put this in was that I didn't think that changes to the default test framework should cause tests to break -- since this test relies on a particular configuration, I felt it was best to define that locally.
There was a problem hiding this comment.
Also, can you set a custom timewait for the RPC connections in this test to resolve the travis issues that have come up so far:
https://travis-ci.org/bitcoin/bitcoin/jobs/248398018#L2153
There was a problem hiding this comment.
We might as well just get rid of the default value. There is nothing special about "4" that justifies to use it as default value.
There was a problem hiding this comment.
ok, you've convinced me. I'll leave this in for now.
We can remove the default num_nodes in a future PR (and probably remove the default topology and premine while we're at it).
test/functional/dbcrash.py
Outdated
There was a problem hiding this comment.
I think this RemoteDisconnected exception may not exist pre-3.5, which would explain this travis failure:
https://travis-ci.org/bitcoin/bitcoin/jobs/248398016#L3321
I'm not sure what the best way to handle that is...
There was a problem hiding this comment.
I think I've fixed this in my fixup commit b394a00f6c34e20e5ff555c8a07c6558e319a34a. Just catch different errors depending on which version of Python is being run.
I think later we should move this logic to a separate method named send_rpc_may_fail() or similar in TestNode to keep the test script code clean.
test/functional/dbcrash.py
Outdated
There was a problem hiding this comment.
Feel free to address the TODO as well :) I think you just do something like relayfee = self.nodes[0].getnetworkinfo()['relayfee'], and then convert to the right units.
There was a problem hiding this comment.
I haven't addressed this. What are the downsides to using a fixed fee?
b7870b7 to
b394a00
Compare
|
I think I've fixed the Travis issues, although it's difficult to tell for sure since Travis only runs extended tests in master. For the first, I split the generate call in Both of these solutions should be moved into TestNode once that's merged (ie a I haven't addressed the fixed fee todo. I don't think that's too much of a problem, but let me know if you think it could cause issues. One question: I'm confused about the randomness in re-org'ing. As far as I can see, the way this works:
That throws up events with very low probabilities. For example, we'll re-org back 40 blocks in 1/40 * 1/44 = 1/1760 tests. For me, such indeterminateness is troubling. If there was a problem that caused large re-orgs to fail, then they'd only be caught very intermittently by this test, and it'd be difficult to debug. I prefer my tests to be more deterministic where possible. How would you feel about changing the re-orgs to a fixed schedule (ie make sure the test covers re-orgs of 1, 2, 4, 8, 16 and 32 blocks, or some similar schedule)? |
|
utACK b394a00f6c34e20e5ff555c8a07c6558e319a34a. Feel free to squash. |
|
squashed |
|
Going to merge this now, since it might fix the test failures we see on master right now. Further cleanup can always be completed in later commits. |
27c63dc [tests] nits in dbcrash.py (John Newbery) Tree-SHA512: 2a75feeb65e6147e3337200cde982248bea8977a9585d5ee284d62bbc25f6d7c368754da0083aec37338c8f66cf698ee25bbd9e192df14a9fb976b8f75afa986
@jnewbery I don't think the right way to think of this test is that we're testing reorgs. Really, we're testing a more complex state (utxo cache + disk state) and trying to get as much coverage of various edge cases as we can. Since it's hard for me to imagine all the possible states we could be in, I wanted to simulate as much normal looking bitcoin transaction volume as I reasonably could, and then randomly stress it with reorgs and crashes in the hopes that if an edge case had a bug, we'd eventually see it. If I knew exactly how to enumerate all the edge cases, then I'd happily convert this to a deterministic set of tests, but lacking that I think the randomness here is an appropriate test strategy. |
ACK |
27c63dc [tests] nits in dbcrash.py (John Newbery) Tree-SHA512: 2a75feeb65e6147e3337200cde982248bea8977a9585d5ee284d62bbc25f6d7c368754da0083aec37338c8f66cf698ee25bbd9e192df14a9fb976b8f75afa986
27c63dc [tests] nits in dbcrash.py (John Newbery) Tree-SHA512: 2a75feeb65e6147e3337200cde982248bea8977a9585d5ee284d62bbc25f6d7c368754da0083aec37338c8f66cf698ee25bbd9e192df14a9fb976b8f75afa986
27c63dc [tests] nits in dbcrash.py (John Newbery) Tree-SHA512: 2a75feeb65e6147e3337200cde982248bea8977a9585d5ee284d62bbc25f6d7c368754da0083aec37338c8f66cf698ee25bbd9e192df14a9fb976b8f75afa986
@sdaftuar - I never reviewed the dbcrash.py test in #10148 . This PR fixes up a few nits:
timeindirectly, no need for try-except importinghttplibsince that's a python 2/3 compatibility trick, and our tests always run in Py3)low-priority, should be fairly uncontroversial, but let me know if you disagree with any of these.