Reenable some broken tests as discussed #20744 (review) and #20744 (review)
Fix windows test cases broken in #20744, by passing normalized path arguments to fs::equivalent, fs::exists, and fs::is_directory, instead of non-normalized arguments. Also re-enable the tests.
It is possible these changes also fix real init behavior on windows when -datadir or -walletdir paths with trailing dots or dashes are used, but it’s not clear because I only tested on wine.
This should also fix an assert error if a -datadir with a trailing slash
is used on windows. This appears to be a real error and regression
introduced with #20744.
On windows (or at least wine), fs calls that actuallly access the
filesystem like fs::equivalent or fs::exists seem to treat directory
paths with trailing slashes as not existing, so it's necessary to
normalize these paths before using them. This fix adds a
path::lexically_normal() call to the failing assert so it passes.
This should also fix an init error if a -walletdir with a trailing slash
is used on windows. This appears to be a real error and regression
introduced with #20744.
On windows (or at least wine), fs calls that actuallly access the
filesystem like fs::equivalent or fs::exists seem to treat directory
paths with trailing slashes as not existing, so it's necessary to
normalize these paths before using them. This change passes canonical
paths to fs calls validating the -walletdir path to fix this.
72@@ -73,8 +73,6 @@ BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_no_trailing)
73 BOOST_CHECK_EQUAL(walletdir, expected_path);
74 }
75
76-#ifndef WIN32
77-// Windows does not consider "datadir/wallets//" to be a valid directory path.
78 BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_no_trailing2)
79 {
80 SetWalletDir(m_walletdir_path_cases["trailing2"]);
Is this going to cause any read or write outside the m_root_path?
I don’t think so, but I’m also not sure where the question is coming from. Wouldn’t you expect invalid paths to trigger errors, not to read or write to completely different locations?
Updated 8287489e7c26ecd434bbf07af8e6aa64f41f9bca -> d216bc8d76d7f4e9dce58b0bb732a2d4deaf23b6 (pr/wp.1 -> pr/wp.2, compare) hopefully fixing CI errors.
I think #20744 might have actually caused real regressions on windows. If -datadir or -walletdir values with trailing slashes are passed, it triggers assert errors or init errors in these two test cases, and it seems like the same problems would happen outside of the test environment.
Not good to disable tests like this!
If
-datadiror-walletdirvalues with trailing slashes are passed, it triggers assert errors or init errors…
You’re talking about Windows builds only, right?
If
-datadiror-walletdirvalues with trailing slashes are passed, it triggers assert errors or init errors…You’re talking about Windows builds only, right?
You left off the important part where I said “… in these two test cases.” And I actually only know about wine, not real windows. With wine, I saw fs::exists and similar functions treat directory paths with trailing dots and slashes as if they did not exist. Normalizing or canonicalizing the arguments made these functions work again and let me re-enable these windows tests. I think adding back test coverage here, and making minor changes to make code more reliable are already good things to do, and would have improved #20744. But it also hints that #20744 could also break some real windows configurations. I don’t know for sure.
ACK d216bc8d76d7f4e9dce58b0bb732a2d4deaf23b6, I have reviewed the code and it looks OK, I agree it can be merged.
Also note that #24265 makes all paths (-datadir, -blocksdir or -walletdir) normalized at the earliest available stage that effectively makes changes in src/util/system.cpp and src/wallet/load.cpp redundant.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
Labels
Wallet