test: Embed univalue json tests in binary #31542
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/31542. 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. ConflictsReviewers, 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. |
DrahtBot
changed the title
|
Nice, Concept ACK |
|
Concept ACK. Thank you for picking this up! |
sedited
left a comment
sedited
left a comment
There was a problem hiding this comment.
ACK fabda022536c7a000a575bbb05059d27d29b5cca
maflcko
force-pushed
the
2412-univalue-json-tests
branch
from
fabda02 to
faf23a8
Compare
hebasto
left a comment
hebasto
left a comment
There was a problem hiding this comment.
ACK faf23a8d0328f288d64dd697de4efedbc6970531.
maflcko
force-pushed
the
2412-univalue-json-tests
branch
from
faf23a8 to
faedcc5
Compare
|
Thx, fixed typo (good catch)! Also, pushed again for |
sedited
left a comment
sedited
left a comment
There was a problem hiding this comment.
Nice improvements, Re-ACK faedcc5fdabc508aaade9b461e44ea3a6670f36b
l0rinc
left a comment
l0rinc
left a comment
There was a problem hiding this comment.
Thanks for the cleanup, please consider a few simplification and unification recommendations
src/univalue/CMakeLists.txt
Outdated
There was a problem hiding this comment.
does this sorting (here and in unitester.cpp) serve some purpose of could we use natural ordering?
We can't see this way that e.g. fail43 is missing and adding new lines would get confusing this way (maybe even adding a 0 prefix for <10):
Details
if(BUILD_TESTS)
include(GenerateHeaders)
generate_header_from_json(test/fail1.json)
generate_header_from_json(test/fail2.json)
generate_header_from_json(test/fail3.json)
generate_header_from_json(test/fail4.json)
generate_header_from_json(test/fail5.json)
generate_header_from_json(test/fail6.json)
generate_header_from_json(test/fail7.json)
generate_header_from_json(test/fail8.json)
generate_header_from_json(test/fail9.json)
generate_header_from_json(test/fail10.json)
generate_header_from_json(test/fail11.json)
generate_header_from_json(test/fail12.json)
generate_header_from_json(test/fail13.json)
generate_header_from_json(test/fail14.json)
generate_header_from_json(test/fail15.json)
generate_header_from_json(test/fail16.json)
generate_header_from_json(test/fail17.json)
generate_header_from_json(test/fail18.json)
generate_header_from_json(test/fail19.json)
generate_header_from_json(test/fail20.json)
generate_header_from_json(test/fail21.json)
generate_header_from_json(test/fail22.json)
generate_header_from_json(test/fail23.json)
generate_header_from_json(test/fail24.json)
generate_header_from_json(test/fail25.json)
generate_header_from_json(test/fail26.json)
generate_header_from_json(test/fail27.json)
generate_header_from_json(test/fail28.json)
generate_header_from_json(test/fail29.json)
generate_header_from_json(test/fail30.json)
generate_header_from_json(test/fail31.json)
generate_header_from_json(test/fail32.json)
generate_header_from_json(test/fail33.json)
generate_header_from_json(test/fail34.json)
generate_header_from_json(test/fail35.json)
generate_header_from_json(test/fail36.json)
generate_header_from_json(test/fail37.json)
generate_header_from_json(test/fail38.json)
generate_header_from_json(test/fail39.json)
generate_header_from_json(test/fail40.json)
generate_header_from_json(test/fail41.json)
generate_header_from_json(test/fail42.json)
generate_header_from_json(test/fail44.json)
generate_header_from_json(test/fail45.json)
generate_header_from_json(test/pass1.json)
generate_header_from_json(test/pass2.json)
generate_header_from_json(test/pass3.json)
generate_header_from_json(test/pass4.json)
generate_header_from_json(test/round1.json)
generate_header_from_json(test/round2.json)
generate_header_from_json(test/round3.json)
generate_header_from_json(test/round4.json)
generate_header_from_json(test/round5.json)
generate_header_from_json(test/round6.json)
generate_header_from_json(test/round7.json)
add_executable(unitester
${CMAKE_CURRENT_BINARY_DIR}/test/fail1.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail2.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail3.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail4.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail5.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail6.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail7.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail8.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail9.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail10.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail11.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail12.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail13.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail14.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail15.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail16.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail17.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail18.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail19.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail20.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail21.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail22.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail23.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail24.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail25.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail26.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail27.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail28.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail29.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail30.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail31.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail32.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail33.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail34.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail35.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail36.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail37.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail38.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail39.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail40.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail41.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail42.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail44.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/fail45.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/pass1.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/pass2.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/pass3.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/pass4.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/round1.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/round2.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/round3.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/round4.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/round5.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/round6.json.h
${CMAKE_CURRENT_BINARY_DIR}/test/round7.json.h
test/unitester.cpp
)There was a problem hiding this comment.
does this sorting (here and in unitester.cpp) serve some purpose of could we use natural ordering?
Yes, the purpose is to follow the clang-format sorting. If it was changed, clang-format will undo the change the next time.
maybe even adding a 0 prefix for <10
Happy to review a follow-up doing this, but I'll leave this as-is for now.
src/univalue/test/unitester.cpp
Outdated
There was a problem hiding this comment.
I understand why a macro is the smallest change to make runtest happy, but I think we can avoid that weird string parsing and introduce enums instead:
enum class TestType { Fail, Pass, RoundTrip };and define the test as:
inline constexpr std::map<std::string_view, TestType> tests{
{json_tests::fail1, TestType::Fail},
{json_tests::fail2, TestType::Fail},
{json_tests::fail3, TestType::Fail},
...which could simplify runtest to:
static void runtest(std::string_view data, TestType type) {
UniValue val;
bool testResult = val.read(std::string{data});
if (type == TestType::Fail) {
assert(!testResult);
return;
}
...There was a problem hiding this comment.
I want to keep the changes minimal. Also, I am not sure if an enum class is the right choice, so I'll leave this as-is for now.
hebasto
left a comment
hebasto
left a comment
There was a problem hiding this comment.
re-ACK faedcc5fdabc508aaade9b461e44ea3a6670f36b.
|
(assigned milestone, because this is a blocker to run the tests on native windows, which would be nice to finally get running) |
Use character literals instead of integer hex values (i.e. `'\x5b','\x0a', ...` instead of `0x5b, 0x0a, ...`) for generated headers. This avoids C++11 narrowing warnings in a more concise way than using explicit char casts. Extra whitespace is also removed between elements for brevity.
|
ACK faedcc5fdabc508aaade9b461e44ea3a6670f36b Avoiding file reads during test setup is a step forward in portability 👍 |
Also, extend the pass2.json test to the maximum depth possible. The two tests are now similar to fail45.json and pass4.json, except for the string element in the inner-most array. Also, sort.
maflcko
force-pushed
the
2412-univalue-json-tests
branch
from
faedcc5 to
faf7eac
Compare
|
ACK faf7eac The changes since last ACK: simplify char casting and reformat the affected sources. |
|
tACK faf7eac Reviewed code and verified that tests are still running as intended. |
|
ACK faf7eac |
All other benchmarks and tests have their data embedded, except for the univalue json tests.
This is not only confusing, but also problematic, when the test binary is moved to a different system for testing, because one has to put the test files in the source dir that was used at compile-time.
Fix all issues by embedding them. Also, re-enable a disabled test. Also, fix an issue in the GenerateHeaderFromJson.cmake.
Requested in https://github.com/bitcoin/bitcoin/pull/31434/files#r1876000910