test: Avoid collision with valid path names in `getarg_tests/logargs` #26473

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:221108-logargs changing 1 files +2 −2

hebasto commented at 7:05 PM on November 8, 2022: member

This PR prevents test failure when "private" is a part of a valid path.

For example, /private/var is a valid path on macOS for temporary files, which in turn causes test failure on CI for tests managed by the CTest framework.
hebasto added the label Tests on Nov 8, 2022
hebasto commented at 7:09 PM on November 8, 2022: member

Friendly ping @LarryRuane (as a test author) and @ryanofsky (as an ArgsManager expert).
LarryRuane commented at 10:31 PM on November 8, 2022: contributor

tACK 400fa45d5df0d5c90d246ce3f0cf5281b69827b7 It would also be okay if, instead of dynamic random data, we just used a very unlikely (but still fixed) string. Thanks for fixing this!

in src/test/getarg_tests.cpp:433 in 400fa45d5d outdated

 429 | @@ -428,8 +430,9 @@ BOOST_AUTO_TEST_CASE(logargs)
 430 |      const auto okaylog_negbool = std::make_pair("-okaylog-negbool", ArgsManager::ALLOW_ANY);
 431 |      const auto okaylog = std::make_pair("-okaylog", ArgsManager::ALLOW_ANY);
 432 |      const auto dontlog = std::make_pair("-dontlog", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE);
 433 | +    const auto private_data = strprintf("%04x", GetRand<uint16_t>());

maflcko commented at 6:48 AM on November 9, 2022:

Why does this need to be random? Given that the test already failed on one platform, how is this not going to introduce a non-deterministic intermittent issue on the same platform (or others)?

hebasto commented at 9:35 AM on November 9, 2022:

Thanks! Updated.

test: Avoid collision with valid path names in `getarg_tests/logargs` c8f91478c1
hebasto force-pushed on Nov 9, 2022
hebasto renamed this:
~~test: Randomize data in `getarg_tests/logargs` test~~
test: Avoid collision with valid path names in `getarg_tests/logargs`
on Nov 9, 2022
hebasto commented at 9:34 AM on November 9, 2022: member

Updated 400fa45d5df0d5c90d246ce3f0cf5281b69827b7 -> c8f91478c10affdfba8e52200c85379052f4e1b1 (pr26473.01 -> pr26473.02, diff).

It would also be okay if, instead of dynamic random data, we just used a very unlikely (but still fixed) string.

Done.
maflcko commented at 11:14 AM on November 9, 2022: member

ACK c8f91478c10affdfba8e52200c85379052f4e1b1
maflcko merged this on Nov 9, 2022
maflcko closed this on Nov 9, 2022
hebasto deleted the branch on Nov 9, 2022
sidhujag referenced this in commit 9296034a34 on Nov 9, 2022
bitcoin locked this on Nov 9, 2023

Contributors

Labels

Linked (view graph)

#25797 build: Add CMake-based build system