Two small test changes:
SeedRand::FIXED_SEED.TEST_DIR_PATH_ELEMENT, and to change its value to better match the TMPDIR_PREFIX in functional tests. The value previously included PACKAGE_NAME, which is cute, but doesn’t explain why it was used (to include a space). So just use test_common bitcoin to achieve the same with less effort.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hodlinator, ryanofsky |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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.
74@@ -77,6 +75,7 @@ const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
75
76 /** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */
77 static FastRandomContext g_rng_temp_path;
78+constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes a space to catch possible path escape issues.
constexpr inline is used to abide by the One Definition Rule when putting variables in header files. This is a .CPP file.
Also would have added an extra newline to separate it from the commented code-section above.
0
1constexpr auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes a space to catch possible path escape issues.
I don’t think there is a risk in putting inline in a CPP file. In fact, it seems easier to just be consistent when using it in headers and CPP files. Otherwise, issues could appear when the code is moved, making it harder to move the code.
The newline was omitted on purpose, to group the two into a related section about the test tmp path.
So I’ll leave this as-is for now.
TEST_DIR_PATH_ELEMENT is self-explanatory, but I am open to consider to include a comment on TEST_DIR_PATH_ELEMENT, if you have a suggestion.
I agree it’s self-explanatory, and it is closely related, but the comment does not apply to it. How about putting it first, still grouping them together?
0constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes a space to catch possible path escape issues.
1/** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */
2static FastRandomContext g_rng_temp_path;
Concept ACK fa9c3aecb18e68cfaeae44aa0daffec020ca3927
Not entirely sure why the two changes are combined in one PR but they are small enough and both touch setup_common.cpp.
Good to see SeedRandomForTest no longer defaults which enum value to use (it was explicitly passed everywhere anyway). :+1:
No remaining references to PACKAGE_NAME in tests, nor “test_common_Bitcoin Core” in project. Shortening to “test_common bitcoin” seems like it should be tolerable for forks.
ACK fa84f9decd224ea1c25dc7095bad70a48fa1a534
git range-diff master fa9c3aec fa84f9de
Only one minor change (that I suggested) since prior review.
Passed ctest -j <cores> --test-dir build.
Code review ACK fa84f9decd224ea1c25dc7095bad70a48fa1a534
IMO, TEST_DIR_PATH_ELEMENT would be clearer as TESTS_DIR_NAME and "test_common bitcoin" would be clearer as "bitcoin tests" but current names seem fine too.
IMO,
TEST_DIR_PATH_ELEMENTwould be clearer asTESTS_DIR_NAMEand"test_common bitcoin"would be clearer as"bitcoin tests"but current names seem fine too.
Looks like #30737 may or may not change the path layout, so any clarifications could in theory be done there.
Labels
Tests