test: remove duplicated code in test/functional/wallet_migration.py #33122

pull kevkevinpal wants to merge 1 commits into bitcoin:master from kevkevinpal:relativePathFollowups changing 1 files +6 −27

kevkevinpal commented at 10:42 pm on August 1, 2025: contributor

This is a followup to #32273

This removes duplicated code in test/functional/wallet_migration.py 6a7c0d3 addresses https://github.com/bitcoin/bitcoin/pull/32273/files#r2237317368
DrahtBot added the label Wallet on Aug 1, 2025
DrahtBot commented at 10:42 pm on August 1, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33122.

Reviews

See the guideline for information on the review process.

Type Reviewers

ACK ryanofsky, l0rinc, jonatack

Concept ACK pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
kevkevinpal renamed this:
~~wallet: remove use of GetWalletDIr and weakly_canonical when creating backup_prefix~~
wallet: remove use of GetWalletDir and weakly_canonical when creating backup_prefix
on Aug 1, 2025

Type	Reviewers
ACK	ryanofsky, l0rinc, jonatack
Concept ACK	pablomartin4btc

in test/functional/wallet_migration.py:625 in 9c2abe57f8 outdated

641-
642-        assert_equal(bals, wallet.getbalances())
643-
644-    def test_wallet_with_path_ending_in_relative_specifier(self):
645+    def test_wallet_with_path(self, wallet_path):
646         self.log.info("Test migrating a wallet with a name/path ending in a relative specifier, '..'")

pablomartin4btc commented at 11:16 pm on August 1, 2025:

nit: perhaps you can add the wallet_path in the log? otherwise it would be seen as duplicated…

kevkevinpal commented at 11:31 pm on August 1, 2025:

yea sure thing updated in 6ac5a351c99f0c0f1b80d179fc845fa55973c874

02025-08-01T23:29:43.011985Z TestFramework (INFO): Test migrating a wallet with the following path/name: path/to/mywallet/
12025-08-01T23:29:43.072030Z TestFramework (INFO): Test migrating a wallet with the following path/name: path/that/ends/in/..

pablomartin4btc commented at 11:16 pm on August 1, 2025: member

Concept ACK
kevkevinpal force-pushed on Aug 1, 2025

in src/wallet/wallet.cpp:4207 in 6ac5a351c9 outdated

4210-    const std::string backup_prefix = wallet_name.empty() ? "default_wallet" : [&] {
4211-        // fs::weakly_canonical resolves relative specifiers and remove trailing slashes.
4212-        const auto legacy_wallet_path = fs::weakly_canonical(GetWalletDir() / fs::PathFromString(wallet_name));
4213-        return fs::PathToString(legacy_wallet_path.filename());
4214+    // on walletname but expanded to "default_wallet" if it is empty and only
4215+    // the final filename portion is used if contains slashes.

maflcko commented at 10:03 am on August 2, 2025:

walletname -> wallet name [compound term should be two words for clarity]
if contains slashes -> if it contains slashes [missing pronoun “it”]

kevkevinpal commented at 2:10 pm on August 2, 2025:

thank you! I’ve updated them in d1b71e5

kevkevinpal force-pushed on Aug 2, 2025
ryanofsky commented at 5:30 pm on August 4, 2025: contributor

I like the test simplification in fa9f30f509ed0ba6e4afdcf98775acb4e8c9e461 but think it would be best to drop commit d1b71e5846e125f59cd60468f2d76c9721b1dc06 for now since @davidgumberg disagreed with this approach in #32273 (comment) it wouldn’t be good to override his preference while he’s out.

I do think d1b71e5846e125f59cd60468f2d76c9721b1dc06 is a minor improvement but there shouldn’t be any need to rush it. And if we do make this change it would be good to accompany it with a test. (A potential test could create a <datadir>/wallets/mywallet symlink pointing to /path/to/my/wallet and verify that the backup prefix is “mywallet_” not “wallet_”.)
test: refactor to remove duplicated test code 6a7c0d3f87
kevkevinpal force-pushed on Aug 4, 2025
kevkevinpal commented at 6:09 pm on August 4, 2025: contributor

6a7c0d3

Thanks! I did not see that comment

I dropped that commit and pushed 6a7c0d3 to just include the functional test change for this PR!
kevkevinpal renamed this:
~~wallet: remove use of GetWalletDir and weakly_canonical when creating backup_prefix~~
test: remove duplicated code in test/functional/wallet_migration.py
on Aug 4, 2025
DrahtBot renamed this:
~~test: remove duplicated code in test/functional/wallet_migration.py~~
test: remove duplicated code in test/functional/wallet_migration.py
on Aug 4, 2025
ryanofsky approved
ryanofsky commented at 7:56 pm on August 5, 2025: contributor

Code review ACK 6a7c0d3f874942a460bf5b13a107a2b21eb2e572. Nice test deduplication, thanks for following on this!
DrahtBot requested review from pablomartin4btc on Aug 5, 2025
l0rinc commented at 8:45 pm on August 5, 2025: contributor

code review ACK 6a7c0d3f874942a460bf5b13a107a2b21eb2e572

in test/functional/wallet_migration.py:625 in 6a7c0d3f87

643-
644-    def test_wallet_with_path_ending_in_relative_specifier(self):
645-        self.log.info("Test migrating a wallet with a name/path ending in a relative specifier, '..'")
646-        wallet_ending_in_relative = "path/that/ends/in/.."
647+    def test_wallet_with_path(self, wallet_path):
648+        self.log.info("Test migrating a wallet with the following path/name: %s", wallet_path)

jonatack commented at 9:46 pm on August 5, 2025:

(nit, only if you have to retouch) generally I believe we’ve been preferring to use f-strings for new python code

0        self.log.info(f"Test migrating a wallet with the following path/name: {wallet_path}")

jonatack commented at 9:50 pm on August 5, 2025: member

ACK 6a7c0d3f874942a460bf5b13a107a2b21eb2e572
fanquake merged this on Aug 6, 2025
fanquake closed this on Aug 6, 2025

Contributors
kevkevinpal DrahtBot pablomartin4btc maflcko ryanofsky l0rinc jonatack

Review Requested
pablomartin4btc

Labels
Wallet

test: remove duplicated code in test/functional/wallet_migration.py #33122

Code Coverage & Benchmarks

Reviews