test: Update wait_until usage in tests not to use the one from utils by slmtpz · Pull Request #19752 · bitcoin/bitcoin

slmtpz · 2020-08-17T18:19:13Z

Replace global (from test_framework/util.py) wait_until() usages with the ones provided by BitcoinTestFramework and P2PInterface classes.

The motivation behind this change is that the util.wait_until() expects a timeout, timeout_factor and lock and it is not aware of the context of the test framework. BitcoinTestFramework offers a wait_until() which has an understandable amount of default timeout and a shared timeout_factor. Moreover, on top of these, mininode.wait_until() also has a shared lock.

closes #19080

maflcko · 2020-08-18T14:26:31Z

Very nice. Concept ACK

practicalswift · 2020-08-18T17:32:01Z

Nice first-time contribution! Warm welcome as a contributor @slmtpz :)

Concept ACK

slmtpz · 2020-08-18T17:35:32Z

Nice first-time contribution! Warm welcome as a contributor @slmtpz :)

Concept ACK

Thank you, looking forward to more contributions in future!

DrahtBot · 2020-08-20T20:08:17Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: Replace -zapwallettxes with wallet tool command #19700 (wallet: Replace -zapwallettxes with wallet tool command by achow101)
wallet: Remove -zapwallettxes #19671 (wallet: Remove -zapwallettxes by achow101)

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.

glozow

Concept ACK, welcome! 😊 this is awesome!

I complain about this pretty often 😅 The util wait_until function doesn't have any context from the TestFramework instance (i.e. timeout and timeout_factor). Sometimes when it's used, the test writer forgets to pass in the mininode_lock which is even worse. I think it would be a good idea to add a comment to the wait_until function in test_framework.util that recommends using something else, because it's almost never the best option - this is just my opinion though. (Btw, I recommend adding something along these lines to the PR description to explain what motivates these changes).

We actually have more options than just TestFramework wait_until. There are a few places where one of the mininode helper functions (wait_for_disconnect, wait_for_broadcast, etc) or the mininode wait_until would be more appropriate.

There seem to be some changes that are unrelated to changing which wait_until is used. I didn't look into them too closely, but could you give an explanation and/or put them in a separate PR?

Also a style nit: there's a few places where you're refactoring the imports in addition to removing wait_until, which I don't think is the style convention and makes the diff larger than it needs to be.

test/functional/p2p_leak.py

glozow · 2020-08-21T03:12:18Z

test/functional/p2p_compactblocks.py

Hm, is there a reason for removing mininode_lock and adding check_connected=False?

Also, wait_for_disconnect would be better.

Used wait_for_disconnect instead, thanks!

glozow · 2020-08-21T03:13:34Z

test/functional/p2p_sendheaders.py

why are you removing the timeout and lock here?

This class extends P2PInterface not BitcoinTestFramework. mininode one already has a lock. As for timeout, it already has a default value of 60.

glozow · 2020-08-21T03:13:43Z

test/functional/p2p_sendheaders.py

This class extends P2PInterface not BitcoinTestFramework. mininode one already has a lock. As for timeout, it already has a default value of 60.

glozow · 2020-08-21T03:26:12Z

test/functional/p2p_compactblocks.py

I'm not sure if this is one of the goals of this PR, but in this situation I think it's best to use test_node.wait_until().

Same with the rest of the test.

I was inclined to use the one from BitcoinTestFramework since I thought it would be good to keep consistency. However, in this case, since we are waiting for a change in test_node, using that one makes more sense, did I get it right? :)

glozow · 2020-08-21T03:30:44Z

test/functional/wallet_resendwallettransactions.py

Here, wait_for_broadcast(txid) would be more appropriate

(txid needs to not be converted to int though)

It looks cleaner now! Please have a look.

glozow · 2020-08-21T03:30:55Z

test/functional/wallet_resendwallettransactions.py

test/functional/rpc_invalidateblock.py

slmtpz · 2020-08-25T19:40:31Z

Concept ACK, welcome! 😊 this is awesome!

I complain about this pretty often 😅 The util wait_until function doesn't have any context from the TestFramework instance (i.e. timeout and timeout_factor). Sometimes when it's used, the test writer forgets to pass in the mininode_lock which is even worse. I think it would be a good idea to add a comment to the wait_until function in test_framework.util that recommends using something else, because it's almost never the best option - this is just my opinion though. (Btw, I recommend adding something along these lines to the PR description to explain what motivates these changes).

We actually have more options than just TestFramework wait_until. There are a few places where one of the mininode helper functions (wait_for_disconnect, wait_for_broadcast, etc) or the mininode wait_until would be more appropriate.

There seem to be some changes that are unrelated to changing which wait_until is used. I didn't look into them too closely, but could you give an explanation and/or put them in a separate PR?

Also a style nit: there's a few places where you're refactoring the imports in addition to removing wait_until, which I don't think is the style convention and makes the diff larger than it needs to be.

Thanks for the thorough review and double thanks for your suggestions! Going deeper, things had started to build up in my mind.
Rebase + force pushed the changes you suggested. As for your other proposal to add a comment in util.wait_until(), please look at this second commit.
PR description is also updated. Should I update commit body as well (after your approval of the motivation for this change stated in the PR description) ?

kallewoof

Big concept ACK: much cleaner!

utACK 0a2d50714f056955ac92f01346bbdc8d5c3019c7

One thing that tripped me up when reviewing is that there are two separate wait_until methods, one (in P2PInterface) which has a default lock (mininode_lock) and one (in BitcoinTestFramework) which doesn't. This is why you see the lock=mininode_lock in a few places, while it's removed in a bunch of others. (Check super class.)

maflcko

ACK

8823fd6c702f3c7c97218b1dbeda2b9386873d69

test/functional/example_test.py

maflcko · 2020-08-26T06:26:09Z

test/functional/feature_versionbits_warning.py

Suggested change

self.wait_until(lambda: not node.getblockchaininfo()['initialblockdownload'], timeout=10, lock=mininode_lock)

self.wait_until(lambda: not node.getblockchaininfo()['initialblockdownload'], timeout=10)

why the lock?

The class of this method inherits from BitcoinTestFramework which does not have a default lock.

but why is the lock needed? Why does it need to be taken here?

I frankly don't know. It was there before, so I didn't touch it.

test/functional/p2p_segwit.py

test/functional/p2p_sendheaders.py

test/functional/p2p_tx_download.py

Replace "wait_until()" usage from utils, with the ones from BitcoinTestFramework and P2PInterface. closes bitcoin#19080

kallewoof

utACK d841301

maflcko · 2020-08-27T06:21:38Z

Thanks for keeping this updated and addressing all the feedback!

ACK d841301 🦆

Show signature and timestamp

Signature:

Timestamp of file with hash 8841ab10d7ee7a298b7cd8d05a44bfb9c3aaaa6e2c48b70e1d0adeb3afe2f62e -

…e the one from utils d841301 test: Add docstring to wait_until() in util.py to warn about its usage (Seleme Topuz) 1343c86 test: Update wait_until usage in tests not to use the one from utils (Seleme Topuz) Pull request description: Replace global (from [test_framework/util.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/util.py#L228)) `wait_until()` usages with the ones provided by `BitcoinTestFramework` and `P2PInterface` classes. The motivation behind this change is that the `util.wait_until()` expects a timeout, timeout_factor and lock and it is not aware of the context of the test framework. `BitcoinTestFramework` offers a `wait_until()` which has an understandable amount of default `timeout` and a shared `timeout_factor`. Moreover, on top of these, `mininode.wait_until()` also has a shared lock. closes bitcoin#19080 ACKs for top commit: MarcoFalke: ACK d841301 🦆 kallewoof: utACK d841301 Tree-SHA512: 81604f4cfa87fed98071a80e4afe940b3897fe65cf680a69619a93e97d45f25b313c12227de7040e19517fa9c003291b232f1b40b2567aba0148f22c23c47a88

Summary: Using the global wait_until makes it impossible to adjust the timeout based on the hardware the test is running on. > test: Update wait_until usage in tests not to use the one from utils > > Replace "wait_until()" usage from utils, with the ones from BitcoinTestFramework and P2PInterface. > closes #19080 > test: Add docstring to wait_until() in util.py to warn about its usage This is a backport of [[bitcoin/bitcoin#19752 | core#19752]] Test Plan: `ninja check-functional-extended` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10128

DrahtBot added the Tests label Aug 17, 2020

DrahtBot added the Needs rebase label Aug 20, 2020

jnewbery mentioned this pull request Aug 20, 2020

test: Remove confusing mininode terminology #19760

Merged

slmtpz force-pushed the master branch from 963c9ce to c3e1a30 Compare August 20, 2020 07:25

DrahtBot removed the Needs rebase label Aug 20, 2020

This was referenced Aug 20, 2020

wallet: Replace -zapwallettxes with wallet tool command #19700

Closed

wallet: Remove -zapwallettxes #19671

Merged

glozow reviewed Aug 21, 2020

View reviewed changes

slmtpz force-pushed the master branch from c3e1a30 to 7a66c78 Compare August 25, 2020 18:21

maflcko reviewed Aug 25, 2020

View reviewed changes

test/functional/rpc_invalidateblock.py Outdated Show resolved Hide resolved

slmtpz force-pushed the master branch 4 times, most recently from 662636e to c132bc3 Compare August 25, 2020 19:32

slmtpz requested a review from maflcko August 25, 2020 19:46

slmtpz force-pushed the master branch from 64dea33 to 0a2d507 Compare August 25, 2020 21:32

DrahtBot mentioned this pull request Aug 26, 2020

test/refactor: reference p2p objects explicitly and remove confusing Test_Node.p2p property #19804

Merged

kallewoof reviewed Aug 26, 2020

View reviewed changes

maflcko approved these changes Aug 26, 2020

View reviewed changes

slmtpz force-pushed the master branch from 0a2d507 to 0639169 Compare August 26, 2020 06:51

DrahtBot added the Needs rebase label Aug 26, 2020

slmtpz added 2 commits August 26, 2020 18:01

test: Update wait_until usage in tests not to use the one from utils

1343c86

Replace "wait_until()" usage from utils, with the ones from BitcoinTestFramework and P2PInterface. closes bitcoin#19080

test: Add docstring to wait_until() in util.py to warn about its usage

d841301

slmtpz force-pushed the master branch from 0639169 to d841301 Compare August 26, 2020 16:06

slmtpz requested a review from kallewoof August 26, 2020 16:07

slmtpz requested a review from maflcko August 26, 2020 16:08

DrahtBot removed the Needs rebase label Aug 26, 2020

kallewoof reviewed Aug 26, 2020

View reviewed changes

maflcko merged commit 28f4e53 into bitcoin:master Aug 27, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

	self.wait_until(lambda: not node.getblockchaininfo()['initialblockdownload'], timeout=10, lock=mininode_lock)
	self.wait_until(lambda: not node.getblockchaininfo()['initialblockdownload'], timeout=10)

Conversation

slmtpz commented Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Aug 18, 2020

Uh oh!

practicalswift commented Aug 18, 2020

Uh oh!

slmtpz commented Aug 18, 2020

Uh oh!

DrahtBot commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slmtpz commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

slmtpz commented Aug 17, 2020 •

edited

Loading

DrahtBot commented Aug 20, 2020 •

edited

Loading

slmtpz commented Aug 25, 2020 •

edited

Loading