test: consolidate to f-strings (part 1) #22229

fanquake commented at 7:53 AM on June 12, 2021: member

Rather than using 3 different ways to build/format strings (sometimes all in the same test, i.e feature_config_args.py), consolidate to using f-strings (3.6+), which are generally more concise / readable, as well as more performant than existing methods.

This deals with the feature_*.py, interface_*.py and mining_*.py tests.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22567 (test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke)
#20362 (test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke)

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.

theStack commented at 10:01 PM on June 12, 2021: member

Concept ACK

DrahtBot added the label Needs rebase on Jun 13, 2021

mjdietzx commented at 9:45 PM on June 13, 2021: contributor

Concept ACK and good idea. Are you using a tool (flynt?) to do this? I guess it requires a good amount of review since there are some cases where there are discrepancies between string formats. I’ll trying running flynt and diffing it against this PR to see that in the next day or so, and give this some more review. I may also trying diffing test logs before/after these changes. Will get back after that. Any thoughts on squashing commits?

josibake commented at 11:46 AM on August 17, 2021: member

Concept ACK

good GOHIO (get our house in order) PR

Zero-1729 commented at 12:09 PM on August 17, 2021: contributor

Concept ACK

in test/functional/feature_backwards_compatibility.py:393 in 705d503a8b outdated

 389 | @@ -390,7 +390,7 @@ def run_test(self):
 390 |              node_master.loadwallet("u1_v17")
 391 |              wallet = node_master.get_wallet_rpc("u1_v17")
 392 |              info = wallet.getaddressinfo(address)
 393 | -            descriptor = "wpkh([" + info["hdmasterfingerprint"] + hdkeypath[1:] + "]" + pubkey + ")"
 394 | +            descriptor = f"wpkh([{info['hdmasterfingerprint']}{hdkeypath[1:]}]{pubkey })"

mjdietzx commented at 1:25 PM on August 17, 2021:

extra space {pubkey }

fanquake commented at 4:39 AM on August 18, 2021:

Fixed

mjdietzx approved

mjdietzx commented at 1:52 PM on August 17, 2021: contributor

Code review ACK, went through line by line, wasn't as much/bad as I expected

Also ran tests and scrolled through (skimming them) and didn't see anything weird that would indicate a malformed string.

jamesob commented at 7:51 PM on August 17, 2021: member

Concept ACK

test: use f-strings in feature_asmap.py 6f3d5ad67a

test: use f-strings in feature_backwards_compatibility.py 5453e87062

test: use f-strings in feature_blocksdir.py dca173cc04

test: use f-strings in feature_cltv.py 36d33d32b1

test: use f-strings in feature_config_args.py e2f1fd8ee9

test: use f-strings in feature_csv_activation.py 3e2f84e7a9

test: use f-strings in feature_dbcrash.py a2502cc63f

test: use f-strings in feature_dersig.py a2de33cbdc

test: use f-strings in feature_fee_estimation.py d5a6adc5e4

test: use f-strings in feature_filelock.py ff7e330999

test: use f-strings in feature_help.py e9ca8b254d

test: use f-strings in feature_loadblock.py fb633933ab

test: use f-strings in feature_logging.py 6679eceacc

test: use f-strings in feature_minchainwork.py 1a546e6f6c

test: use f-strings in feature_notifications.py 961f5813ba

test: use f-strings in feature_pruning.py 6651d77f22

test: use f-strings in feature_settings.py cf6d66bf94

test: use f-strings in feature_versionbits_warning.py b166d54c3c

test: use f-strings in feature_segwit.py 31bdb33dcb

test: use f-strings in feature_proxy.py 86d958262d

test: use f-strings in interface_*.py tests c2a5d560df

test: use f-strings in mining_*.py tests 68faa87881

fanquake force-pushed on Aug 18, 2021

fanquake commented at 4:40 AM on August 18, 2021: member

Rebased, addressed comment, and squashed these down a bit.

DrahtBot removed the label Needs rebase on Aug 18, 2021

mjdietzx commented at 7:30 AM on August 18, 2021: contributor

reACK 68faa87881f5334b2528db4adc72ec19d94316a3

Zero-1729 commented at 8:55 AM on August 18, 2021: contributor

crACK 68faa87881f5334b2528db4adc72ec19d94316a3

MarcoFalke merged this on Aug 18, 2021

MarcoFalke closed this on Aug 18, 2021

in test/functional/interface_rest.py:117 in 68faa87881

 113 | @@ -114,7 +114,7 @@ def run_test(self):
 114 |          assert_equal(self.nodes[1].getbalance(), Decimal("0.1"))
 115 |  
 116 |          # Check chainTip response
 117 | -        json_obj = self.test_rest_request("/getutxos/{}-{}".format(*spending))
 118 | +        json_obj = self.test_rest_request(f"/getutxos/{spending[0]}-{spending[1]}")

MarcoFalke commented at 7:48 PM on August 18, 2021:

While f-strings are preferred, I think here it is subjective which version is better. Since our dev notes don't require a specific style, I think it is fine to not use f-strings everywhere.

MarcoFalke commented at 7:51 PM on August 18, 2021: member

Also, I'd slightly prefer if the conflicts (https://github.com/bitcoin/bitcoin/pull/22229#issuecomment-860075392) are first dealt with before part 2 (etc) are opened.

fanquake deleted the branch on Aug 18, 2021

sidhujag referenced this in commit bf979c9a68 on Aug 20, 2021

DrahtBot locked this on Aug 19, 2022