Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jul 11, 2020

Calling minonode.wait_until needs a connection to make progress (e.g. waiting for an inv), unless the mininode waits for the initial connection or for a disconnection. So for test development and failure debugging, fail early in all wait_until, unless opted out.

@fanquake fanquake added the Tests label Jul 11, 2020
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 11, 2020

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

Conflicts

Reviewers, 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.

@maflcko
Copy link
Member Author

maflcko commented Jul 30, 2020

cc @jnewbery Mind taking a look here? 🙏

Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK faa9a74.

One suggestion inline. Feel free to take or ignore.

return self.message_count["verack"]

self.wait_until(test_function, timeout=timeout)
self.wait_until(test_function, timeout=timeout, check_connected=False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be improved even more by making add_p2p_connection() wait until is_connected is True before returning. That way you could leave check_connected as True for wait_for_verack() as well as in p2p_filter.py

@maflcko maflcko merged commit 3c93623 into bitcoin:master Aug 4, 2020
@maflcko maflcko deleted the 2007-testFailEarly branch August 4, 2020 09:23
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 4, 2020
faa9a74 test: Fail wait_until early if connection is lost (MarcoFalke)

Pull request description:

  Calling `minonode.wait_until` needs a connection to make progress (e.g. waiting for an inv), unless the mininode waits for the initial connection or for a disconnection. So for test development and failure debugging, fail early in all `wait_until`, unless opted out.

ACKs for top commit:
  jnewbery:
    Code review ACK faa9a74.

Tree-SHA512: 4be850b96e23b87bc2ff42c028a5045d6f5cdbc9482ce6a6ba01cc5eb26710dab9e2ed547c363aac4bd5825151ee9996fb797261420b631bceeddbfa698d1dec
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 13, 2021
Summary:
faa9a74c9e99eb43ba0d27fa906767ee88011aeb test: Fail wait_until early if connection is lost (MarcoFalke)

Pull request description:

  Calling `minonode.wait_until` needs a connection to make progress (e.g. waiting for an inv), unless the mininode waits for the initial connection or for a disconnection. So for test development and failure debugging, fail early in all `wait_until`, unless opted out.

---

Depends on D9514

Backport of [[bitcoin/bitcoin#19489 | core#19489]]

Test Plan:
  ninja all check check-functional

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D9515
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants