test: makes timeout a forced named argument in tests methods that use it #29750
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK 985d4d4 |
This makes calls to such methods more explicit and less error prone
985d4d4 to
61560d5
Compare
|
lgtm ACK 61560d5 |
| self.wait_for_getheaders(timeout=timeout) | ||
| self.send_message(msg) | ||
| self.wait_for_getdata([block.sha256]) | ||
| self.wait_for_getdata([block.sha256], timeout=timeout) |
There was a problem hiding this comment.
Nit: default timeout=60, but that's 60 secs for each inner function that takes a timeout, so the total timeout for announce_block_and_wait_for_getdata is 120 in this case.
Probably fine, but potentially confusing, i.e. announce_block_and_wait_for_getdata(..., timeout=60) can actually run for ~120 seconds. An alternative would be something like timeout=timeout//2 on wait_for_getheaders and wait_for_getdata but that doesn't seem much better to me. More complex could be to subtract 'already spent time' from timeout after each inner wait_* call?
There was a problem hiding this comment.
Also nit: should timeout on announce_block_and_wait_for_getdata also be kwargs only?
There was a problem hiding this comment.
Nit: default timeout=60, but that's 60 secs for each inner function that takes a timeout, so the total timeout for announce_block_and_wait_for_getdata is 120 in this case.
the first 60 second timeout would cause the test to crash and we wouldn't wait around for the next crash to happen :)
Also nit: should timeout on announce_block_and_wait_for_getdata also be kwargs only?
don't think it's worth it since it's only a local function used in 1 file.
There was a problem hiding this comment.
the first 60 second timeout would cause the test to crash and we wouldn't wait around for the next crash to happen :)
I meant if the first took say 59s to succeed then the second another 59s, but yeah it's not really an issue
There was a problem hiding this comment.
Nit: default timeout=60, but that's 60 secs for each inner function that takes a timeout, so the total timeout for announce_block_and_wait_for_getdata is 120 in this case.
the first 60 second timeout would cause the test to crash and we wouldn't wait around for the next crash to happen :)
Also nit: should timeout on announce_block_and_wait_for_getdata also be kwargs only?
don't think it's worth it since it's only a local function used in 1 file.
Yeah, this was my intuition too. The same applies to the total timeout, I think it'd be worth if this was used outside the context of this file, but it is actually only used in a single test
| self.wait_for_getheaders(timeout=timeout) | ||
| self.send_message(msg) | ||
| self.wait_for_getdata([block.sha256]) | ||
| self.wait_for_getdata([block.sha256], timeout=timeout) |
There was a problem hiding this comment.
Nit: default timeout=60, but that's 60 secs for each inner function that takes a timeout, so the total timeout for announce_block_and_wait_for_getdata is 120 in this case.
the first 60 second timeout would cause the test to crash and we wouldn't wait around for the next crash to happen :)
Also nit: should timeout on announce_block_and_wait_for_getdata also be kwargs only?
don't think it's worth it since it's only a local function used in 1 file.
|
rfm? |
This makes calls to such methods more explicit and less error-prone.
Motivated by #29736 (comment)