test: Replace (dis)?connect_nodes globals with TestFramework methods #19967
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Sep 17, 2020
fb556cb to
cfd2512
Compare
Contributor
Author
|
Since many existing files contain an import that looks like this: 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. |
cfd2512 to
d169a11
Compare
ghost
approved these changes
Sep 17, 2020
ghost
left a comment
There was a problem hiding this comment.
ACK 85fa2ec9ac5db68d7d38567c70ce2d7587fc7487
I have reviewed it and it looks okay.
ghost
approved these changes
Sep 17, 2020
ghost
left a comment
There was a problem hiding this comment.
ACK bef7f3b8a0934153cf94a6cbb809e5353d73f84c
Reviewed it and changes made in two files look okay.
ccfbc86 to
2f62edc
Compare
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
This was referenced Sep 19, 2020
Contributor
|
concept ACK! |
2f62edc to
18e6ed0
Compare
guggero
reviewed
Oct 4, 2020
Contributor
guggero
left a comment
There was a problem hiding this comment.
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.
Member
|
@robot-dreams Are you still working on this? |
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".
-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>
18e6ed0 to
3c7d9ab
Compare
guggero
reviewed
Oct 21, 2020
Member
|
ACK 3c7d9ab |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 21, 2020
…estFramework methods 3c7d9ab test: Move (dis)?connect_nodes globals into TestFramework as helpers (Elliott Jin) 4b16c61 scripted-diff: test: Replace uses of (dis)?connect_nodes global (Prayank) be38684 test: Replace use of (dis)?connect_nodes globals (Elliott Jin) Pull request description: `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 bitcoin#19945 but uses a scripted-diff for the majority of the changes. Fixes: bitcoin#19821 ACKs for top commit: MarcoFalke: ACK 3c7d9ab guggero: ACK 3c7d9ab Tree-SHA512: e027092748602904abcd986d7299624c8754c3236314b6d8e392e306741c212f266c2207e385adfb194f67ae6559a585ee7b15d639b1d65c4651dbf503e5931a
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Nov 29, 2021
Summary: This is a semi-automatic regex text replacement (excluding files in the `test_framework/` directory): `(dis)?connect_nodes\(self\.nodes\[(\d)\], self\.nodes\[(\d)\]\)` `-->` `self.$1connect_nodes($2, $3)` This is a backport of [[bitcoin/bitcoin#19967 | core#19967]] [2/3] bitcoin/bitcoin@4b16c61 Test Plan: `ninja check-functional-extended` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10559
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Nov 29, 2021
Summary: There is now only one callsite for these functions, so we can just move the code where it is needed. I deviated from the original commit by not creating the `..._helper` functions. It doesn't make much sense to define local functions that are used only once and immediately after being defined. This is a backport of [[bitcoin/bitcoin#19967 | core#19967]] [3/3] bitcoin/bitcoin@3c7d9ab Depends on D10558 and D10559 Test Plan: `ninja check-functional-extended` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10560
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
util.pydefines global helper functionsconnect_nodesanddisconnect_nodes; however, these functions are confusing because they take a node and an index (instead of two indexes).The
TestFrameworkobject has enough context to convert fromitoself.nodes[i], so we can replace all instances ofconnect_nodes(self.nodes[a], b)withself.connect_nodes(a, b). Similarly, we can replace instances ofdisconnect_nodes.The approach taken in this PR builds on #19945 but uses a scripted-diff for the majority of the changes.
Fixes: #19821