qa: Drop recursive deletes from test code, add lint checks. by davidgumberg · Pull Request #34439 · bitcoin/bitcoin

davidgumberg · 2026-01-29T03:59:52Z

Both fs::remove_all and shutil::rmtree() are a bit dangerous, and most of their uses are not necessary, this PR removes most instances of both.

remove_all() is still used in in src/test/util/setup_common.cpp as part of BasicTestingSetup::BasicTestingSetup's constructor and destructor, and it is used in the kernel test code's TestDirectory:

bitcoin/src/test/kernel/test_kernel.cpp

Lines 100 to 112 in 734899a

    
           struct TestDirectory { 
        
               std::filesystem::path m_directory; 
        
               TestDirectory(std::string directory_name) 
        
                   : m_directory{std::filesystem::temp_directory_path() / (directory_name + random_string(16))} 
        
               { 
        
                   std::filesystem::create_directories(m_directory); 
        
               } 
        
               ~TestDirectory() 
        
               { 
        
                   std::filesystem::remove_all(m_directory); 
        
               } 
        
           };

In both cases, remove_all is likely necessary, but the kernel's test code is RAII, ideally BasicTestingSetup could be made similar in a follow-up or in this PR if reviewers think it is important.

Similarly in the python code, most usage was unnecessary, but there are a few places where rmtree() was necessary, I have added sanity checks to make sure these are inside of the tmpdir before doing recursive delete there.

DrahtBot · 2026-01-29T04:00:25Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	l0rinc
Approach ACK	w0xlt

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34603 (wallet: Fix detection of symlinks on Windows by achow101)
#34418 (qa: Make wallet_multiwallet.py Windows crossbuild-compatible by hodlinator)
#34372 (QA: wallet_migration: Test several more weird scenarios by luke-jr)

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.

DrahtBot · 2026-01-29T04:05:10Z

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21465095038/job/61825339241}
_{LLM reason (✨ experimental): Lint check failed due to Ruff error: unused import shutil in test/functional/test_framework/test_node.py.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

l0rinc · 2026-01-29T10:38:27Z

Sounds reasonable, light concept ACK

w0xlt

Approach ACK

sedited

lgtm

The changes to the cache directory logic in the last two commits were not that straight forward to understand for me. Can you elaborate on them a bit in the commit messages?

davidgumberg · 2026-03-10T23:33:55Z

The changes to the cache directory logic in the last two commits were not that straight forward to understand for me. Can you elaborate on them a bit in the commit messages?

Thanks for the feedback, I've (hopefully) improved the commit message for the one where I drop the --keepcache argument, and I've moved changes that I accidentally included in the last commit in the series back to their true home in that commit.

Adds a lint check for `remove_all()` `fs::remove_all()`/`std::filesystem::remove_all()` is extremely dangerous, all user-facing instances of it have been removed, and it also deserves to be removed from the places in our test code where it is being used unnecessarily.

At the time this was added in bitcoin#10197, building the test cache took 21 seconds, as described in that PR, but this is no longer true, as demonstrated by running the functional test framework with and without the --keepcache arguments on master prior to this commit: ``` hyperfine --warmup 1 --export-markdown results.md --runs 3 \ -n 'without --keepcache' './build/test/functional/test_runner.py -j $(nproc)' \ -n 'with --keepcache' './build/test/functional/test_runner.py -j $(nproc) --keepcache' ``` | Command | Mean [s] | Min [s] | Max [s] | Relative | |:----------------------|---------------:|--------:|---:|---:| | `without --keepcache` | 76.373 ± 3.058 | 74.083 | 79.846 | 1.00 | | `with --keepcache` | 77.384 ± 1.836 | 75.952 | 79.454 | 1.01 ± 0.05 | As a consequence, this argument can be removed from the test runner and this also has the benefit of being able to use an RAII-like `tempfile.TemporaryDirectory` instead of having to clean up the cache manually at the end of test runs. bitcoin#10197: bitcoin#10197

`shutil.rmtree` is dangerous because it recursively deletes. There are not likely to be any issues with it's current uses, but it is possible that some of the assumptions being made now won't always be true, e.g. about what some of the variables being passed to `rmtree` represent. For some remaining uses of rmtree that can't be avoided for now, use `cleanup_dir` which asserts that the recursively deleted folder is a child of the the `tmpdir` of the test run. Otherwise, `tempfile.TemporaryDirectory` should be used which does it's own deleting on being garbage collected, or old fashioned unlinking and rmdir in the case of directories with known contents.

DrahtBot · 2026-03-13T23:45:52Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the Tests label Jan 29, 2026

davidgumberg force-pushed the 2026-01-28-remove-all branch from 734899a to 4f83671 Compare January 29, 2026 04:04

DrahtBot added the CI failed label Jan 29, 2026

DrahtBot removed the CI failed label Jan 29, 2026

w0xlt reviewed Jan 29, 2026

View reviewed changes

This was referenced Jan 29, 2026

qa: Make wallet_multiwallet.py Windows crossbuild-compatible #34418

Merged

QA: wallet_migration: Test several more weird scenarios #34372

Open

refactor: Use util::Result class for wallet loading #25722

Draft

maflcko mentioned this pull request Feb 4, 2026

coins: remove redundant and confusing CCoinsViewDB::HaveCoin #34320

Open

DrahtBot mentioned this pull request Feb 6, 2026

kernel, refactor: return error status on all fatal errors #29700

Draft

DrahtBot mentioned this pull request Feb 17, 2026

wallet: Fix detection of symlinks on Windows #34603

Draft

DrahtBot mentioned this pull request Mar 1, 2026

kernel: Use fs:: namespace and unicode path in kernel tests #34705

Merged

sedited approved these changes Mar 8, 2026

View reviewed changes

DrahtBot added the Needs rebase label Mar 9, 2026

hodlinator mentioned this pull request Mar 10, 2026

guix: Make guix-clean more careful #34776

Merged

davidgumberg force-pushed the 2026-01-28-remove-all branch from 4f83671 to bba180f Compare March 10, 2026 23:32

DrahtBot added CI failed and removed Needs rebase labels Mar 10, 2026

davidgumberg added 3 commits March 12, 2026 12:56

davidgumberg force-pushed the 2026-01-28-remove-all branch from bba180f to 548dc81 Compare March 12, 2026 19:56

DrahtBot removed the CI failed label Mar 12, 2026

maflcko mentioned this pull request Mar 13, 2026

wallet, test: Ancient Wallet Migration from v0.14.3 (no-HD and Single Chain) #33186

Open

DrahtBot added the Needs rebase label Mar 13, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qa: Drop recursive deletes from test code, add lint checks.#34439

qa: Drop recursive deletes from test code, add lint checks.#34439
davidgumberg wants to merge 3 commits intobitcoin:masterfrom
davidgumberg:2026-01-28-remove-all

davidgumberg commented Jan 29, 2026 •

edited

Loading

Uh oh!

DrahtBot commented Jan 29, 2026 •

edited

Loading

Uh oh!

DrahtBot commented Jan 29, 2026

Uh oh!

l0rinc commented Jan 29, 2026

Uh oh!

w0xlt left a comment

Uh oh!

sedited left a comment

Uh oh!

davidgumberg commented Mar 10, 2026

Uh oh!

DrahtBot commented Mar 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	struct TestDirectory {
	std::filesystem::path m_directory;
	TestDirectory(std::string directory_name)
	: m_directory{std::filesystem::temp_directory_path() / (directory_name + random_string(16))}
	{
	std::filesystem::create_directories(m_directory);
	}

	~TestDirectory()
	{
	std::filesystem::remove_all(m_directory);
	}
	};

Conversation

davidgumberg commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

DrahtBot commented Jan 29, 2026

Uh oh!

l0rinc commented Jan 29, 2026

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

davidgumberg commented Mar 10, 2026

Uh oh!

DrahtBot commented Mar 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

davidgumberg commented Jan 29, 2026 •

edited

Loading

DrahtBot commented Jan 29, 2026 •

edited

Loading