Skip to content

test: Replace global wait_until with BitcoinTestFramework.wait_until and mininode.wait_until #19134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
dboures wants to merge 2 commits into from
Closed

test: Replace global wait_until with BitcoinTestFramework.wait_until and mininode.wait_until #19134

dboures wants to merge 2 commits into from

Conversation

@dboures
Copy link

@dboures dboures commented Jun 1, 2020 •
edited
Loading

Closes: #19080

This PR creates a wait_until method for the BitcoinTestFramework class, and replaces all uses of the global wait_until with the class method. If applicable, the mininode wait_until is used instead.

@DrahtBot DrahtBot added the Tests label Jun 1, 2020
maflcko
maflcko reviewed Jun 1, 2020
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

Concept ACK

@dboures dboures marked this pull request as draft June 1, 2020 22:13
glozow
glozow reviewed Jun 2, 2020
View reviewed changes
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

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

Concept ACK
In keeping with the goal of this PR, I also recommend changing the wait in feature_abortnode.py here to use wait_until_node_stopped so that it uses the test_node timeout factor.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 4, 2020 •
edited
Loading

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.

@DrahtBot DrahtBot mentioned this pull request Jun 5, 2020
Closed
11 tasks
@glozow glozow mentioned this pull request Jun 8, 2020
Closed
@dboures dboures changed the title test: Replace global wait_until with mininode.wait_until test: Replace global wait_until with BitcoinTestFramework.wait_until and mininode.wait_until Jun 8, 2020
@dboures dboures marked this pull request as ready for review June 8, 2020 23:49
This was referenced Jun 9, 2020
Merged
Closed
adamjonas
adamjonas reviewed Jun 11, 2020
View reviewed changes
Copy link
Member

@adamjonas adamjonas left a comment

Choose a reason for hiding this comment

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

Concept ACK. A few questions on when you specify and omit timeouts.

@DrahtBot DrahtBot mentioned this pull request Jun 13, 2020
Merged
This was referenced Jun 27, 2020
Closed
Merged
@maflcko
Copy link
Member

maflcko commented Jul 10, 2020

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

@DrahtBot DrahtBot mentioned this pull request Jul 14, 2020
Merged
@jonatack jonatack mentioned this pull request Jul 19, 2020
Merged
@jonatack
Copy link
Member

jonatack commented Jul 19, 2020

Concept ACK. Needs squash and rebase.

@fjahr
Copy link
Contributor

fjahr commented Jul 19, 2020 •
edited
Loading

Concept ACK

I think you missed test/functional/p2p_ping.py. And in your current commit order you are using the TestFramework wait_until before it is added. But I am not sure if that is viewed as critical (so nit). Otherwise lgtm.

@dboures dboures marked this pull request as draft July 19, 2020 23:39
@dboures dboures closed this Jul 19, 2020
@dboures dboures reopened this Jul 19, 2020
@dboures dboures marked this pull request as ready for review July 19, 2020 23:41
@DrahtBot DrahtBot mentioned this pull request Jul 20, 2020
Merged
maflcko pushed a commit that referenced this pull request Jul 21, 2020
Merge #19552: test: fix intermittent failure in p2p_ibd_txrelay
edfeaf6 
12410b1 test: fix intermittent p2p_ibd_txrelay race, add test_framework.py#wait_until (Jon Atack)

Pull request description:

  To fix these intermittent failures in Travis CI.
  ```
  162/163 - p2p_ibd_txrelay.py failed, Duration: 2 s

  stdout:
  2020-07-19T05:44:17.213000Z TestFramework (INFO):
      Check that nodes set minfilter to MAX_MONEY while still in IBD
  2020-07-19T05:44:17.216000Z TestFramework (ERROR): Assertion failed
  Traceback (most recent call last):
    File "/Users/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin16/test/functional/test_framework/test_framework.py", line 117, in main
      self.run_test()
    File "/Users/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin16/test/functional/p2p_ibd_txrelay.py", line 30, in run_test
      assert_equal(conn_info['minfeefilter'], MAX_FEE_FILTER)
    File "/Users/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-apple-darwin16/test/functional/test_framework/util.py", line 49, in assert_equal
      raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))

  AssertionError: not(0E-8 == 0.09170997)
  2020-07-19T05:44:17.293000Z TestFramework (INFO): Stopping nodes
  ```

  At Marco's suggestion, cherry-picked part of #19134 to nicely simplify using `wait_until`.

ACKs for top commit:
  vasild:
    ACK 12410b1

Tree-SHA512: 615f509883682fd693e578b259cba35a9fa0bc519f1394e88c857e8b0650bfec5397bfa856cfa9e6d5ef81d0ee6ad02e4ad2b0eb0bd530b4c281cbe3e663790b
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 22, 2020

🐙 This pull request conflicts with the target branch and needs rebase.

@dboures dboures marked this pull request as draft July 23, 2020 02:14
This was referenced Jul 31, 2020
Merged
Closed
@maflcko
Copy link
Member

maflcko commented Aug 4, 2020

@dboures Are you still working on this?

@maflcko
Copy link
Member

maflcko commented Aug 15, 2020

Going to close this as "up for grabs". @dboures Feel free to open a new pull request once you have all the code ready for review.

@maflcko maflcko closed this Aug 15, 2020
@maflcko maflcko mentioned this pull request Aug 15, 2020
Closed
@glozow
Copy link
Member

glozow commented Aug 16, 2020

Aw man :( was so excited for this! Hope you come back soon @dboures

@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.

Reviewers

@glozow glozow glozow left review comments

+2 more reviewers

@adamjonas adamjonas adamjonas left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Needs rebase Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

test: Replace gobal wait_until with mininode.wait_until

7 participants

@dboures @DrahtBot @maflcko @jonatack @fjahr @glozow @adamjonas