test: makes timeout a forced named argument in tests methods that use it #29750

pull sr-gi wants to merge 1 commits into bitcoin:master from sr-gi:202403-test-forced-timeout changing 5 files +22 −22

sr-gi commented at 12:42 PM on March 27, 2024: member

This makes calls to such methods more explicit and less error-prone.

Motivated by #29736 (review)
DrahtBot commented at 12:42 PM on March 27, 2024: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK maflcko, brunoerg, BrandonOdiwuor, AngusP, stratospher

Concept ACK kevkevinpal

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #29748 (test: Cleans some manual checks/drops when using wait_for_getdata by sr-gi)
- #29736 (test: Extends wait_for_getheaders so a specific block hash can be checked by sr-gi)
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.
DrahtBot added the label Tests on Mar 27, 2024
kevkevinpal commented at 1:13 PM on March 27, 2024: contributor

Concept ACK 985d4d4
test: makes timeout a forced named argument in tests methods that use it
This makes calls to such methods more explicit and less error prone
61560d5e93
sr-gi force-pushed on Mar 27, 2024
maflcko commented at 7:05 PM on March 27, 2024: member

lgtm ACK 61560d5e939034e1a94d95cdc5c498095ab4fddb
brunoerg approved
brunoerg commented at 10:35 AM on March 28, 2024: contributor

ACK 61560d5e939034e1a94d95cdc5c498095ab4fddb
BrandonOdiwuor commented at 5:36 PM on March 28, 2024: contributor

crACK 61560d5e939034e1a94d95cdc5c498095ab4fddb
in test/functional/p2p_segwit.py:203 in 61560d5e93
```
 197 | @@ -198,15 +198,15 @@ def announce_block_and_wait_for_getdata(self, block, use_header, timeout=60):
 198 |              self.send_message(msg)
 199 |          else:
 200 |              self.send_message(msg_inv(inv=[CInv(MSG_BLOCK, block.sha256)]))
 201 | -            self.wait_for_getheaders()
 202 | +            self.wait_for_getheaders(timeout=timeout)
 203 |              self.send_message(msg)
 204 | -        self.wait_for_getdata([block.sha256])
 205 | +        self.wait_for_getdata([block.sha256], timeout=timeout)
```
AngusP commented at 9:56 PM on March 29, 2024:

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?

AngusP commented at 9:58 PM on March 29, 2024:

Also nit: should timeout on announce_block_and_wait_for_getdata also be kwargs only?

stratospher commented at 7:57 AM on March 30, 2024:

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.

AngusP commented at 10:43 PM on March 31, 2024:

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

sr-gi commented at 7:36 AM on April 2, 2024:

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
AngusP approved
AngusP commented at 9:59 PM on March 29, 2024: contributor

ACK 61560d5e939034e1a94d95cdc5c498095ab4fddb
stratospher commented at 7:57 AM on March 30, 2024: contributor

tested ACK 61560d5.
maflcko commented at 7:16 AM on April 2, 2024: member

rfm?
fanquake merged this on Apr 2, 2024
fanquake closed this on Apr 2, 2024
Pttn referenced this in commit a979ce8ee2 on Jan 10, 2025
bitcoin locked this on Apr 2, 2025