[tests] functional tests should call BitcoinTestFramework start/stop node methods by jnewbery · Pull Request #10359 · bitcoin/bitcoin

jnewbery · 2017-05-08T16:38:59Z

Second step towards #10082

This PR changes the individual test cases to call the BitcoinTestFramework.start_node and .stop_node methods rather than the util.py functions. This is in preparation for moving the start/stop node functionality into BitcoinTestFramework (currently the BitcoinTestFramework start/stop node methods are just wrappers for the util.py functions).

The second commit changes the names of the functions in util.py to mark them as private. This is so no test cases are added/modified to call the functions directly before the next step of #10082 is merged.

maflcko · 2017-05-08T18:17:48Z

Concept ACK. Needs rebase.

jnewbery · 2017-05-15T20:00:16Z

rebased

This commit changes the individual test scripts to call the start_node(s) and stop_node(s) methods in BitcoinTestFramework.

This commit marks the start/stop functions in util.py as private module functions. A future PR will remove these entirely and move the functionality directly into the BitcoinTestFramework class, but setting them as private in this PR will prevent anyone from accidentally calling them before that future PR is merged.

jnewbery · 2017-05-31T21:50:54Z

rebased. It'd be really helpful to get this merged so we can make some progress on #10082 . This is the only disruptive changeset in that larger PR - the rest of the changes are restricted to the test_framework directory.

This PR touches a lot of files, but it should be a trivial review. It's simply changing calls from stop_node() to calls to self.stop_node() etc wrapper functions in the first commit. The second commit renames the inner functions so no-one calls them explicitly from the test cases (the inner functions will be removed entirely in a future PR).

@MarcoFalke , @TheBlueMatt , @ryanofsky , @sdaftuar, @jimmysong, @kallewoof - if I could convince any of you to review, I'd be very grateful!

ryanofsky

Slightly tested ACK a433d8a

ryanofsky · 2017-05-31T22:29:30Z

test/functional/blockchain.py

    assert_raises_jsonrpc,
    assert_is_hex_string,
    assert_is_hash_string,
+    connect_nodes_bi,


Change seems unrelated, maybe revert.

yes. not sure how that snuck in. Reverted.

ryanofsky · 2017-05-31T22:33:56Z

test/functional/fundrawtransaction.py

        ############################################################
        # locked wallet test
        self.nodes[1].encryptwallet("test")
+        self.stop_node(0)


Maybe move these up one line so nodes[1].encrypt and pop(1) can be next to each other.

jimmysong

ACK. testing now.

jimmysong · 2017-05-31T22:27:39Z

test/functional/abandonconflict.py

-        stop_node(self.nodes[0],0)
-        self.nodes[0]=start_node(0, self.options.tmpdir, ["-minrelaytxfee=0.0001"])
+        self.stop_node(0)
+        self.nodes[0] = self.start_node(0, self.options.tmpdir, ["-minrelaytxfee=0.0001"])


The plan is to change start_node so assignment here would be unnecessary in the future?

yes - that's certainly something we can do in a future PR once the BitcoinTestFramework class is responsible for tracking and stop/starting the nodes.

jimmysong · 2017-05-31T22:29:08Z

test/functional/blockchain.py

    assert_raises_jsonrpc,
    assert_is_hex_string,
    assert_is_hash_string,
+    connect_nodes_bi,


Not sure why this was added?

me neither 😳. Removed

jimmysong · 2017-05-31T22:30:14Z

test/functional/fundrawtransaction.py

-        stop_node(self.nodes[2], 3)

-        self.nodes = start_nodes(self.num_nodes, self.options.tmpdir)
+        self.nodes = self.start_nodes(self.num_nodes, self.options.tmpdir)


I assume that self.start_nodes would also do the assignment in the future?

Unless it should be possible to append nodes.

certainly something we can do in a future PR.

BitcoinTestFramework could include two functions - start_nodes() to start the nodes in self.nodes, and add_nodes() if you need to append nodes to that list.

jimmysong · 2017-05-31T22:31:35Z

test/functional/net.py

    connect_nodes_bi,
-    p2p_port,
-)
+    p2p_port)


Nit: I personally like comma endings so adding new imports has cleaner diffs.

Sure. Reverted.

jimmysong · 2017-05-31T23:35:31Z

Tested, ACK a433d8a

A couple of really small nits. Excited to keep this moving!

kallewoof

utACK a433d8a

kallewoof · 2017-06-01T00:04:11Z

test/functional/fundrawtransaction.py

-        stop_node(self.nodes[2], 3)

-        self.nodes = start_nodes(self.num_nodes, self.options.tmpdir)
+        self.nodes = self.start_nodes(self.num_nodes, self.options.tmpdir)


Unless it should be possible to append nodes.

kallewoof · 2017-06-01T00:04:47Z

test/functional/net.py

    connect_nodes_bi,
-    p2p_port,
-)
+    p2p_port)


jnewbery · 2017-06-01T14:08:56Z

Thanks @ryanofsky @jimmysong @kallewoof . Nits fixed in fixup commit. Will squash when this is ready for merge.

maflcko · 2017-06-02T10:12:03Z

utACK 53f6775

…rk start/stop node methods 53f6775 fixup: fix nits (John Newbery) a433d8a [tests] Update start/stop node functions to be private module functions (John Newbery) d8c218f [tests] Functional tests call self.start_node(s) and self.stop_node(s) (John Newbery) Tree-SHA512: 9cc01584a5e57686b7e7cb1c4c5186ad8cc7eb650d6d4f27b06bdb5e249a10966705814bdfb22d9ff2d5d3326911e489bf3d22257d751a299c0b24b7f40bffb5

…Framework start/stop node methods 53f6775 fixup: fix nits (John Newbery) a433d8a [tests] Update start/stop node functions to be private module functions (John Newbery) d8c218f [tests] Functional tests call self.start_node(s) and self.stop_node(s) (John Newbery) Tree-SHA512: 9cc01584a5e57686b7e7cb1c4c5186ad8cc7eb650d6d4f27b06bdb5e249a10966705814bdfb22d9ff2d5d3326911e489bf3d22257d751a299c0b24b7f40bffb5

jnewbery mentioned this pull request May 8, 2017

[tests] Demostration/concept: Add a TestNode class to test_Framework.py #10082

Closed

6 tasks

maflcko added the Tests label May 8, 2017

jnewbery force-pushed the test_framework_start_stop_nodes branch 2 times, most recently from 883081d to fb580db Compare May 9, 2017 13:42

kallewoof mentioned this pull request May 11, 2017

[test] Remove all * imports #10366

Closed

jnewbery force-pushed the test_framework_start_stop_nodes branch from fb580db to e42627d Compare May 15, 2017 20:00

jnewbery added 2 commits May 31, 2017 16:59

[tests] Functional tests call self.start_node(s) and self.stop_node(s)

d8c218f

This commit changes the individual test scripts to call the start_node(s) and stop_node(s) methods in BitcoinTestFramework.

jnewbery force-pushed the test_framework_start_stop_nodes branch from e42627d to a433d8a Compare May 31, 2017 21:00

ryanofsky reviewed May 31, 2017

View reviewed changes

jimmysong approved these changes May 31, 2017

View reviewed changes

kallewoof approved these changes Jun 1, 2017

View reviewed changes

fixup: fix nits

53f6775

maflcko merged commit 53f6775 into bitcoin:master Jun 2, 2017

jnewbery deleted the test_framework_start_stop_nodes branch June 2, 2017 13:06

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

jnewbery commented May 8, 2017

Uh oh!

maflcko commented May 8, 2017

Uh oh!

jnewbery commented May 15, 2017

Uh oh!

jnewbery commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

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!

jimmysong left a comment

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!

jimmysong commented May 31, 2017

Uh oh!

kallewoof left a comment

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!

jnewbery commented Jun 1, 2017

Uh oh!

maflcko commented Jun 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jnewbery commented May 31, 2017 •

edited

Loading