Skip to content

[qa] TestNode: Add wait_until_stopped helper method#11067

Merged
maflcko merged 1 commit intobitcoin:masterfrom
maflcko:Mf1708-qaTestnodeWaitStopHelper
Sep 6, 2017
Merged

[qa] TestNode: Add wait_until_stopped helper method#11067
maflcko merged 1 commit intobitcoin:masterfrom
maflcko:Mf1708-qaTestnodeWaitStopHelper

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Aug 16, 2017 •
edited
Loading

This adds a helper method wait_until_stopped to the TestNode class. This should prevent numerous time.sleep() over all places.

Additionally, the timeout behavior is restored. (Was removed by the introduction of TestNode.)
This should prevent tests from running indefinitely by accident.

@maflcko maflcko added the Tests label Aug 16, 2017
@jnewbery
Copy link
Contributor

jnewbery commented Aug 16, 2017 •
edited
Loading

Concept ACK, but I'm concerned about defining a wait_until in util.py, when there's already a wait_until defined in mininode.py. This is a problem since wildcard imports are used in a lot of tests, so behavior will be unpredictable.

@jnewbery jnewbery mentioned this pull request Aug 16, 2017
Merged
@jnewbery
Copy link
Contributor

jnewbery commented Aug 17, 2017

Rebase on #11068?

@maflcko maflcko force-pushed the Mf1708-qaTestnodeWaitStopHelper branch 3 times, most recently from fa8d9e1 to fa7d395 Compare August 17, 2017 18:04
@maflcko maflcko changed the title [qa] TestNode: Add wait_until_node_stopped helper method [qa] TestNode: Add wait_until_stopped helper method Aug 17, 2017
@maflcko maflcko force-pushed the Mf1708-qaTestnodeWaitStopHelper branch from 2b687c5 to fa7d395 Compare August 23, 2017 21:19
@jnewbery
Copy link
Contributor

jnewbery commented Aug 24, 2017

Looks great. Tested ACK fa7d395048c9395bbf23439c2986751db560aa8b

@maflcko maflcko force-pushed the Mf1708-qaTestnodeWaitStopHelper branch from fa7d395 to fa72a00 Compare August 24, 2017 20:26
@maflcko
Copy link
Member Author

maflcko commented Aug 24, 2017

Solved merge conflict fa7d39504 ... fa72a0011

jnewbery
jnewbery reviewed Aug 25, 2017
View reviewed changes
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.

Tested ACK fa72a0011bead99f60e6442b79ddad407d3a3df2.

One nit inline. Otherwise looks good.

test/functional/test_framework/test_node.py Outdated
Copy link
Contributor

@jnewbery jnewbery Aug 25, 2017

Choose a reason for hiding this comment

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

I think this method should also set self.rpc to None

Copy link
Member Author

@maflcko maflcko Sep 6, 2017

Choose a reason for hiding this comment

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

@jnewbery Done

@jnewbery jnewbery mentioned this pull request Aug 28, 2017
Merged
@maflcko maflcko force-pushed the Mf1708-qaTestnodeWaitStopHelper branch 2 times, most recently from 9e8827c to aa91654 Compare September 6, 2017 16:38
@maflcko maflcko force-pushed the Mf1708-qaTestnodeWaitStopHelper branch from aa91654 to faa8d95 Compare September 6, 2017 16:41
jnewbery
jnewbery reviewed Sep 6, 2017
View reviewed changes
test/functional/test_framework/test_node.py
)
from .authproxy import JSONRPCException

BITCOIND_PROC_WAIT_TIMEOUT = 60
Copy link
Contributor

@jnewbery jnewbery Sep 6, 2017

Choose a reason for hiding this comment

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

This constant is now only used in one place. Perhaps remove it as a constant and add it as an integer literal in the definition of wait_until_stopped() with a comment.

If not, perhaps move this constant assignment down to just above the call to wait_until_stopped()

Copy link
Member Author

@maflcko maflcko Sep 6, 2017

Choose a reason for hiding this comment

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

I prefer to put constants at the top of a file and my personal taste is that it is ugly to place them between or inside methods/functions.

Copy link
Contributor

@jnewbery jnewbery Sep 6, 2017

Choose a reason for hiding this comment

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

That's reasonable

@jnewbery
Copy link
Contributor

jnewbery commented Sep 6, 2017

utACK faa8d95

@maflcko maflcko merged commit faa8d95 into bitcoin:master Sep 6, 2017
maflcko pushed a commit that referenced this pull request Sep 6, 2017
Merge #11067: [qa] TestNode: Add wait_until_stopped helper method
31809d6 
faa8d95 [qa] TestNode: Add wait_until_stopped helper method (MarcoFalke)

Pull request description:

  This adds a helper method `wait_until_stopped` to the `TestNode` class. This should prevent numerous `time.sleep()` over all places.

  Additionally, the timeout behavior is restored. (Was removed by the introduction of `TestNode`.)
  This should prevent tests from running indefinitely by accident.

Tree-SHA512: 7133fc64d55711869c4e372e9d30625c98f1237fb3578c24a26900d9319831f10eb95592d7b08e536fa706158dffb0abf9197f11c5d9ef605c880628e1a6533f
@maflcko maflcko deleted the Mf1708-qaTestnodeWaitStopHelper branch September 6, 2017 18:07
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 3, 2017
[qa] TestNode: Add wait_until_stopped helper method
eeb24a3 
Github-Pull: bitcoin#11067
Rebased-From: faa8d95
codablock pushed a commit to codablock/dash that referenced this pull request Sep 24, 2019
@codablock
Merge bitcoin#11067: [qa] TestNode: Add wait_until_stopped helper method
ded0187 
faa8d95 [qa] TestNode: Add wait_until_stopped helper method (MarcoFalke)

Pull request description:

  This adds a helper method `wait_until_stopped` to the `TestNode` class. This should prevent numerous `time.sleep()` over all places.

  Additionally, the timeout behavior is restored. (Was removed by the introduction of `TestNode`.)
  This should prevent tests from running indefinitely by accident.

Tree-SHA512: 7133fc64d55711869c4e372e9d30625c98f1237fb3578c24a26900d9319831f10eb95592d7b08e536fa706158dffb0abf9197f11c5d9ef605c880628e1a6533f
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jnewbery jnewbery jnewbery left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@maflcko @jnewbery