test: group executed tests within the same directory #31291
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/31291. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
pablomartin4btc
left a comment
pablomartin4btc
left a comment
There was a problem hiding this comment.
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.
hodlinator
left a comment
hodlinator
left a comment
There was a problem hiding this comment.
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.
For that we could introduce a |
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.
furszy
force-pushed
the
2024_test_global_path
branch
from
ce0645c to
b7f40e8
Compare
Maybe at that stage it's acceptable to require unique |
For sure. That wouldn't be hard to do neither. |
hodlinator
left a comment
hodlinator
left a comment
There was a problem hiding this comment.
ACK b7f40e8
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.
furszy
changed the title
| /* Used to group tests executed within the same binary run under the same directory */ | ||
| static const std::string g_init_time{util::ToString(TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now()))}; | ||
|
|
There was a problem hiding this comment.
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.
drwx------ 3 rkrux staff 96B Nov 15 11:09 1731649167593349000
drwx------ 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
drwxr-xr-x 10 rkrux staff 320B Nov 15 12:25 test_runner_?_🏃_20241115_120930
drwxr-xr-x 7 rkrux staff 224B Nov 15 15:12 test_runner_?_🏃_20241115_150614
There was a problem hiding this comment.
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.
|
I think given that different processes (the normal case with |
|
Hmm, okay. Since the inclusion of CTest, we can run unit tests in parallel, but they are executed in standalone processes, which makes this approach impractical. Maybe we could address this at the CTest level, but I don’t have much time to spend on it. |
6 participants
Solving #31000 (comment).
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_timeAfter:
tmp/test_common bitcoin/current_time/test_name