test: fix Windows CI failures in wallet_multiwallet and old binary tests (ancient wallets) #34233
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34233. ReviewsSee the guideline for information on the review process. 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. |
test/functional/test_runner.py
Outdated
| # Create base test directory | ||
| # Use ASCII-only path when running tests with old binaries (e.g. v0.14.3) that | ||
| # don't handle Unicode on Windows. Only affects runs that include such tests. | ||
| old_binary_tests = ('feature_unsupported_utxo_db', 'wallet_multiwallet') |
There was a problem hiding this comment.
I fail to see where wallet_multiwallet loads an old 14.3 binary (or any old binary), so this comment seems to be wrong.
There was a problem hiding this comment.
Well, this is still globally disabling unicode tests on Windows, just to run a single edge-case test feature_unsupported_utxo_db.py. I'd say unicode tests are more important than this test, and this test could trivially be run sequentially after the other tests, if there was need to.
It seems a bit invasive to change the test framework to work around a single prev-releases test on a single platform.
My preference would be to leave this as-is for now, and either leave the test excluded, or just run it sequentially with a tmpdir overridden for just this test.
There was a problem hiding this comment.
Updated test_runner.py to use an ASCII-only tmpdir path specifically for feature_unsupported_utxo_db.py on Windows.
Everything else remains unchanged.
There was a problem hiding this comment.
I mean, there's no change to the Unicode path. The PR still adds both feature_unsupported_utxo_db.py and wallet_multiwallet.py to CI.
Skip several tests that don't work correctly on Windows: 1. chmod-based no_access test: Windows does not support Unix-style file permissions via os.chmod(). Setting chmod(path, 0) doesn't actually make the directory inaccessible. 2. recursive_dir_symlink test: Skip recursive directory symlink test on Windows as directory symlinks behave differently. 3. w8_symlink test: File symlinks behave differently on Windows and don't trigger the expected "Invalid -wallet path" error. 4. Error message check: Use "filesystem error:" for all platforms since the cross-compiled MinGW build produces the same error format as Linux/macOS.
dc3f958 to
ed816f8
Compare
…indows On Windows, use an ASCII-only tmpdir path specifically for feature_unsupported_utxo_db.py, which uses old v0.14.3 binaries that don't handle Unicode paths properly. All other tests continue to use the Unicode path (test_runner_₿_🏃_) to preserve Unicode path testing.
ed816f8 to
548a51a
Compare
|
Note that re-enabling |
| # On Windows, use ASCII-only path for tests that use old binaries (e.g. v0.14.3) | ||
| # which don't handle Unicode paths properly. | ||
| if platform.system() == 'Windows' and test_argv[0] == 'feature_unsupported_utxo_db.py': | ||
| ascii_tmpdir = "{}/test_runner_ascii_{}".format(tempfile.gettempdir(), datetime.datetime.now().strftime("%Y%m%d_%H%M%S")) |
There was a problem hiding this comment.
This will now silently (and without any way to change it), pick a different tmpdirprefix.
I understand that this only happens on Windows, and only for a single test, and only when the previous releases have been downloaded.
However, this feels a bit like a layer violation, and all of this temporary code will have to be maintained and removed later on.
I've implemented my alternative in https://www.github.com/bitcoin/bitcoin/pull/34240/changes/671303af2e2bad2bda992ad783654d06c3a1d36e
There was a problem hiding this comment.
I agree with this assessment. We can move forward with #34240.
|
Closing it in favor of #34240 |
|
Yeah, I haven't looked at the other commit, but it may be fine. Seems fine to re-open this and put in draft for now. Once it conflicts with master, you can just rebase it, and move it out of draft again. |
Currently, the
wallet_multiwallet.pyandfeature_unsupported_utxo_db.pyfunctional tests are disabled on CI because they rely on legacy wallet binaries that do not correctly handle Unicode paths.This is tracked as a
TODOin.github/workflows/ci.yml. The limitation also blocks efforts to expand ancient wallet test coverage (e.g. #33186).This PR restores functional test coverage on Windows CI by addressing two root causes:
Skip Windows-incompatible tests in
wallet_multiwallet: Several test cases depend on Unix-specific filesystem behavior that is not supported on Windows:os.chmod(path, 0))Use ASCII-only tmpdir for old binary tests: When running tests against previous releases (
feature_unsupported_utxo_db,wallet_multiwallet) on Windows, an ASCII-only temporary directory name (test_runner_btc_run_) is used instead oftest_runner_₿_🏃_. This avoids failures caused by older binaries lacking proper Unicode path support.