Solving #31000 (review).
Modifies the default datadir path for unit/bench/fuzz tests, grouping all tests within a single directory labeled by the current timestamp.
Replicating the functional test framework behavior.
Directory structure update:
Before: tmp/test_common bitcoin/test_name/current_time
After: tmp/test_common bitcoin/current_time/test_name
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31291.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hodlinator |
| Concept ACK | rkrux |
| Stale ACK | pablomartin4btc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
ACK ce0645c312d0395627f1497140108cfe91473bc2
Groups all tests executed within each binary call under a single directory prefixed by the current time. Replicating the function test framework behavior.
It makes sense to me. I’ll play a bit with it just to see how it looks.
75@@ -76,6 +76,9 @@ const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
76
77 constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes a space to catch possible path escape issues.
78
79+/* Used to group tests executed within the same binary run under the same directory */
80+static uint64_t g_start_time = TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now());
Global mutable state should be avoided if possible, just for peace of mind.
0static const uint64_t g_start_time = TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now());
Concept ACK ce0645c312d0395627f1497140108cfe91473bc2
Only case I can think of where having time segment before test_name in the path would be annoying is if one was re-running 2 specific tests a variable amount of times. This PR makes it so that one has to “open every door” (time segment) to see if files for test-A was there or for test-B. In general I think this way is a net-positive though.
Good to see the -testdatadir description updated, sorry for not catching it in #31000.
nit: Would prefer current_time replaced with start_time (or process_start_time) in the PR summary.
140@@ -138,8 +141,7 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts)
141
142 const std::string test_name{G_TEST_GET_FULL_NAME ? G_TEST_GET_FULL_NAME() : ""};
143 if (!m_node.args->IsArgSet("-testdatadir")) {
144- const auto now{TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now())};
145- m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / test_name / util::ToString(now);
146+ m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / util::ToString(g_start_time) / test_name;
nit: We are recalculating util::ToString() for the same value every time we pass by this. Could do it once on line 80.
0 m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / g_start_time / test_name;
Only case I can think of where having time segment before test_name in the path would be annoying is if one was re-running 2 specific tests a variable amount of times.
For that we could introduce a -repeat arg, and suffix the dir with the loop round number.
Modifies the unit/bench/fuzz tests default datadir path.
Groups all tests executed within each binary call under
a single directory prefixed by the current time.
Replicating the function test framework behavior.
Only case I can think of where having time segment before test_name in the path would be annoying is if one was re-running 2 specific tests a variable amount of times.
For that we could introduce a
-repeatarg, and suffix the dir with the loop round number.
Maybe at that stage it’s acceptable to require unique -testdatadir being passed in for every run.
Only case I can think of where having time segment before test_name in the path would be annoying is if one was re-running 2 specific tests a variable amount of times.
For that we could introduce a
-repeatarg, and suffix the dir with the loop round number.Maybe at that stage it’s acceptable to require unique
-testdatadirbeing passed in for every run.
For sure. That wouldn’t be hard to do neither.
ACK b7f40e8094e00c13a6c847340645b16704fed63b
Ran ctest -j 20 --test-dir build and build/src/test/test_bitcoin and inspected activity with lsof |grep "/tmp.*bitcoin. The former will create a new process and hence time directory for every test, but run them in parallel. The latter will run all tests sequentially in-process, resulting in only one new time directory.
75@@ -76,6 +76,9 @@ const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
76
77 constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes a space to catch possible path escape issues.
78
79+/* Used to group tests executed within the same binary run under the same directory */
80+static const std::string g_init_time{util::ToString(TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now()))};
81+
Replicating the functional test framework behavior.
Should we use a prettier format here just like done in the functional tests? With this change the directory name is in plain nanoseconds like below.
0drwx------ 3 rkrux staff 96B Nov 15 11:09 1731649167593349000
1drwx------ 3 rkrux staff 96B Nov 15 11:11 1731649280506089000
Whereas for functional tests, the name is a little easier to read by using a date time format: https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_runner.py#L476
0drwxr-xr-x 10 rkrux staff 320B Nov 15 12:25 test_runner_?_🏃_20241115_120930
1drwxr-xr-x 7 rkrux staff 224B Nov 15 15:12 test_runner_?_🏃_20241115_150614
Think of each unit test as a separate child class inheriting from the testing setup class and implementing a run() method. Each one constructs a new instance of the setup class, thereby creating a new rand_str.
One thing that I noticed is that there are several such (1731649167593349000) time directories when the unit tests are run, each having around one test_name directory inside. Whereas for the functional tests, a dir such as test_runner_?_🏃_20241115_150614 encapsulates all of them.
Concept ACK b7f40e8094e00c13a6c847340645b16704fed63b
Successful make and unit tests. Used ctest --test-dir build -j 13 to run the unit tests and checked for the new directory setup in $TMPDIR, which unintuitively on OSX is not /tmp. :)
ctest -j$(nproc)) still create different time dirs, I think it is also fine to leave this as-is. To me this is purely a style issue, so I don’t mind either way.