test: Replace (dis)?connect_nodes globals with TestFramework methods

robot-dreams commented at 7:59 am on September 17, 2020: contributor

util.py defines global helper functions connect_nodes and disconnect_nodes; however, these functions are confusing because they take a node and an index (instead of two indexes).

The TestFramework object has enough context to convert from i to self.nodes[i], so we can replace all instances of connect_nodes(self.nodes[a], b) with self.connect_nodes(a, b). Similarly, we can replace instances of disconnect_nodes.

The approach taken in this PR builds on #19945 but uses a scripted-diff for the majority of the changes.

Fixes: #19821

fanquake added the label Tests on Sep 17, 2020

robot-dreams force-pushed on Sep 17, 2020

robot-dreams commented at 11:13 am on September 17, 2020: contributor

Since many existing files contain an import that looks like this:

0from test_framework.util import (
1    assert_equal,
2)

I think it makes more sense to address these in a separate PR (e.g. with a scripted-diff), instead of trying to manually fix the newly created ones in this PR.

robot-dreams closed this on Sep 17, 2020

robot-dreams reopened this on Sep 17, 2020

robot-dreams force-pushed on Sep 17, 2020

unknown approved

unknown commented at 12:31 pm on September 17, 2020: none

ACK 85fa2ec9ac5db68d7d38567c70ce2d7587fc7487

I have reviewed it and it looks okay.

unknown approved

unknown commented at 12:40 pm on September 17, 2020: none

ACK bef7f3b8a0934153cf94a6cbb809e5353d73f84c

Reviewed it and changes made in two files look okay.

robot-dreams force-pushed on Sep 17, 2020

robot-dreams force-pushed on Sep 18, 2020

DrahtBot commented at 1:26 pm on September 19, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20186 (wallet: Make -wallet setting not create wallets by ryanofsky)
#20074 (test: p2p_blockfilters tests for BIP157 config args by robot-dreams)
#19877 ([test] clarify rpc_net & p2p_disconnect_ban functional tests by amitiuttarwar)
#18788 (tests: Update more tests to work with descriptor wallets by achow101)
#18766 (Disable fee estimation in blocksonly mode (by removing the fee estimates global) by darosior)
#17526 (Use Single Random Draw In addition to knapsack as coin selection fallback 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.

amitiuttarwar commented at 7:27 pm on September 21, 2020: contributor

concept ACK!

glozow commented at 5:19 pm on September 22, 2020: member

ACK https://github.com/bitcoin/bitcoin/pull/19967/commits/2f62edc767e39795cf72cd18fcb49179938d07ac

This is much cleaner, thanks!

DrahtBot added the label Needs rebase on Sep 23, 2020

robot-dreams force-pushed on Oct 2, 2020

DrahtBot removed the label Needs rebase on Oct 2, 2020

in test/functional/rpc_getpeerinfo_deprecation.py:28 in 18e6ed0b37 outdated

25@@ -26,7 +26,7 @@ def test_banscore_deprecation(self):
26 
27     def test_addnode_deprecation(self):
28         self.restart_node(1, ["-deprecatedrpc=getpeerinfo_addnode"])
29-        connect_nodes(self.nodes[0], 1)
30+        self.connect_nodes(0, 1)

guggero commented at 10:55 am on October 4, 2020:

The scripted-diff script seems to have missed the import in this file.

robot-dreams commented at 7:50 am on October 20, 2020:

Thanks for catching this!

guggero commented at 10:58 am on October 4, 2020: contributor

Concept ACK 18e6ed0b.

The scripted-diff script needs adjustment, see comment below. I can confirm the code move in 18e6ed0b doesn’t contain any change in its behavior. In fact the only diff is the logger.warning -> self.log.warning change.

MarcoFalke commented at 1:22 pm on October 19, 2020: member

@robot-dreams Are you still working on this?

test: Replace use of (dis)?connect_nodes globals

A later scripted-diff commit replaces the majority of uses, which all
follow this pattern:

    (dis)?connect_nodes(self.nodes[a], b)

This commit replaces the few "special cases".

be386840d4

scripted-diff: test: Replace uses of (dis)?connect_nodes global

-BEGIN VERIFY SCRIPT-

 # max-depth=0 excludes test/functional/test_framework/...
 FILES=$(git grep -l --max-depth 0 "connect_nodes" test/functional)

 # Replace (dis)?connect_nodes(self.nodes[a], b) with self.(dis)?connect_nodes(a, b)
 sed -i 's/\b\(dis\)\?connect_nodes(self\.nodes\[\(.*\)\]/self.\1connect_nodes(\2/g' $FILES

 # Remove imports in the middle of a line
 sed -i 's/\(dis\)\?connect_nodes, //g' $FILES
 sed -i 's/, \(dis\)\?connect_nodes//g' $FILES

 # Remove imports on a line by themselves
 sed -i '/^\s*\(dis\)\?connect_nodes,\?$/d' $FILES
 sed -i '/^from test_framework\.util import connect_nodes$/d' $FILES

-END VERIFY SCRIPT-

Co-authored-by: Elliott Jin <elliott.jin@gmail.com>

4b16c61461

test: Move (dis)?connect_nodes globals into TestFramework as helpers 3c7d9ab8c8

robot-dreams force-pushed on Oct 20, 2020

guggero commented at 12:31 pm on October 21, 2020: contributor

ACK 3c7d9ab8

MarcoFalke commented at 12:42 pm on October 21, 2020: member

ACK 3c7d9ab8c8ec5284cdad1a53ee310b79b931f12f

MarcoFalke merged this on Oct 21, 2020

MarcoFalke closed this on Oct 21, 2020

robot-dreams deleted the branch on Oct 21, 2020

sidhujag referenced this in commit fa2305a135 on Oct 21, 2020

Fabcien referenced this in commit 00ae313d30 on Nov 29, 2021

Fabcien referenced this in commit d87a16fbc2 on Nov 29, 2021

DrahtBot locked this on Feb 15, 2022

test: Replace (dis)?connect_nodes globals with TestFramework methods #19967

Conflicts