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

2025-08-01T23:29:43.011985Z TestFramework (INFO): Test migrating a wallet with the following path/name: path/to/mywallet/
2025-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

        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

Review Requested
pablomartin4btc

Labels

Wallet

Linked (view graph)

#32273 wallet: Fix relative path backup during migration.

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

Code Coverage & Benchmarks

Reviews