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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31542.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | l0rinc |
| Stale ACK | TheCharlatan, hebasto |
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.
19- target_compile_definitions(unitester
20- PRIVATE
21- JSON_TEST_SRC=\"${CMAKE_CURRENT_SOURCE_DIR}/test\"
22- )
23+ include(GenerateHeaders)
24+ generate_header_from_json(data/base58_encode_decode.json)
57+ generate_header_from_json(test/fail40.json)
58+ generate_header_from_json(test/fail41.json)
59+ generate_header_from_json(test/fail42.json)
60+ generate_header_from_json(test/fail44.json)
61+ generate_header_from_json(test/fail45.json)
62+ generate_header_from_json(test/fail3.json)
131+ ${CMAKE_CURRENT_BINARY_DIR}/test/round3.json.h
132+ ${CMAKE_CURRENT_BINARY_DIR}/test/round4.json.h
133+ ${CMAKE_CURRENT_BINARY_DIR}/test/round5.json.h
134+ ${CMAKE_CURRENT_BINARY_DIR}/test/round6.json.h
135+ ${CMAKE_CURRENT_BINARY_DIR}/test/round7.json.h
136+ test/unitester.cpp)
style nit: Follow surrounding style:
0 test/unitester.cpp
1 )
?
inline constexpr :sweat_smile:
19- target_compile_definitions(unitester
20- PRIVATE
21- JSON_TEST_SRC=\"${CMAKE_CURRENT_SOURCE_DIR}/test\"
22+ include(GenerateHeaders)
23+ generate_header_from_json(test/fail1.json)
24+ generate_header_from_json(test/fail10.json)
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):
0if(BUILD_TESTS)
1 include(GenerateHeaders)
2 generate_header_from_json(test/fail1.json)
3 generate_header_from_json(test/fail2.json)
4 generate_header_from_json(test/fail3.json)
5 generate_header_from_json(test/fail4.json)
6 generate_header_from_json(test/fail5.json)
7 generate_header_from_json(test/fail6.json)
8 generate_header_from_json(test/fail7.json)
9 generate_header_from_json(test/fail8.json)
10 generate_header_from_json(test/fail9.json)
11 generate_header_from_json(test/fail10.json)
12 generate_header_from_json(test/fail11.json)
13 generate_header_from_json(test/fail12.json)
14 generate_header_from_json(test/fail13.json)
15 generate_header_from_json(test/fail14.json)
16 generate_header_from_json(test/fail15.json)
17 generate_header_from_json(test/fail16.json)
18 generate_header_from_json(test/fail17.json)
19 generate_header_from_json(test/fail18.json)
20 generate_header_from_json(test/fail19.json)
21 generate_header_from_json(test/fail20.json)
22 generate_header_from_json(test/fail21.json)
23 generate_header_from_json(test/fail22.json)
24 generate_header_from_json(test/fail23.json)
25 generate_header_from_json(test/fail24.json)
26 generate_header_from_json(test/fail25.json)
27 generate_header_from_json(test/fail26.json)
28 generate_header_from_json(test/fail27.json)
29 generate_header_from_json(test/fail28.json)
30 generate_header_from_json(test/fail29.json)
31 generate_header_from_json(test/fail30.json)
32 generate_header_from_json(test/fail31.json)
33 generate_header_from_json(test/fail32.json)
34 generate_header_from_json(test/fail33.json)
35 generate_header_from_json(test/fail34.json)
36 generate_header_from_json(test/fail35.json)
37 generate_header_from_json(test/fail36.json)
38 generate_header_from_json(test/fail37.json)
39 generate_header_from_json(test/fail38.json)
40 generate_header_from_json(test/fail39.json)
41 generate_header_from_json(test/fail40.json)
42 generate_header_from_json(test/fail41.json)
43 generate_header_from_json(test/fail42.json)
44 generate_header_from_json(test/fail44.json)
45 generate_header_from_json(test/fail45.json)
46 generate_header_from_json(test/pass1.json)
47 generate_header_from_json(test/pass2.json)
48 generate_header_from_json(test/pass3.json)
49 generate_header_from_json(test/pass4.json)
50 generate_header_from_json(test/round1.json)
51 generate_header_from_json(test/round2.json)
52 generate_header_from_json(test/round3.json)
53 generate_header_from_json(test/round4.json)
54 generate_header_from_json(test/round5.json)
55 generate_header_from_json(test/round6.json)
56 generate_header_from_json(test/round7.json)
57 add_executable(unitester
58 ${CMAKE_CURRENT_BINARY_DIR}/test/fail1.json.h
59 ${CMAKE_CURRENT_BINARY_DIR}/test/fail2.json.h
60 ${CMAKE_CURRENT_BINARY_DIR}/test/fail3.json.h
61 ${CMAKE_CURRENT_BINARY_DIR}/test/fail4.json.h
62 ${CMAKE_CURRENT_BINARY_DIR}/test/fail5.json.h
63 ${CMAKE_CURRENT_BINARY_DIR}/test/fail6.json.h
64 ${CMAKE_CURRENT_BINARY_DIR}/test/fail7.json.h
65 ${CMAKE_CURRENT_BINARY_DIR}/test/fail8.json.h
66 ${CMAKE_CURRENT_BINARY_DIR}/test/fail9.json.h
67 ${CMAKE_CURRENT_BINARY_DIR}/test/fail10.json.h
68 ${CMAKE_CURRENT_BINARY_DIR}/test/fail11.json.h
69 ${CMAKE_CURRENT_BINARY_DIR}/test/fail12.json.h
70 ${CMAKE_CURRENT_BINARY_DIR}/test/fail13.json.h
71 ${CMAKE_CURRENT_BINARY_DIR}/test/fail14.json.h
72 ${CMAKE_CURRENT_BINARY_DIR}/test/fail15.json.h
73 ${CMAKE_CURRENT_BINARY_DIR}/test/fail16.json.h
74 ${CMAKE_CURRENT_BINARY_DIR}/test/fail17.json.h
75 ${CMAKE_CURRENT_BINARY_DIR}/test/fail18.json.h
76 ${CMAKE_CURRENT_BINARY_DIR}/test/fail19.json.h
77 ${CMAKE_CURRENT_BINARY_DIR}/test/fail20.json.h
78 ${CMAKE_CURRENT_BINARY_DIR}/test/fail21.json.h
79 ${CMAKE_CURRENT_BINARY_DIR}/test/fail22.json.h
80 ${CMAKE_CURRENT_BINARY_DIR}/test/fail23.json.h
81 ${CMAKE_CURRENT_BINARY_DIR}/test/fail24.json.h
82 ${CMAKE_CURRENT_BINARY_DIR}/test/fail25.json.h
83 ${CMAKE_CURRENT_BINARY_DIR}/test/fail26.json.h
84 ${CMAKE_CURRENT_BINARY_DIR}/test/fail27.json.h
85 ${CMAKE_CURRENT_BINARY_DIR}/test/fail28.json.h
86 ${CMAKE_CURRENT_BINARY_DIR}/test/fail29.json.h
87 ${CMAKE_CURRENT_BINARY_DIR}/test/fail30.json.h
88 ${CMAKE_CURRENT_BINARY_DIR}/test/fail31.json.h
89 ${CMAKE_CURRENT_BINARY_DIR}/test/fail32.json.h
90 ${CMAKE_CURRENT_BINARY_DIR}/test/fail33.json.h
91 ${CMAKE_CURRENT_BINARY_DIR}/test/fail34.json.h
92 ${CMAKE_CURRENT_BINARY_DIR}/test/fail35.json.h
93 ${CMAKE_CURRENT_BINARY_DIR}/test/fail36.json.h
94 ${CMAKE_CURRENT_BINARY_DIR}/test/fail37.json.h
95 ${CMAKE_CURRENT_BINARY_DIR}/test/fail38.json.h
96 ${CMAKE_CURRENT_BINARY_DIR}/test/fail39.json.h
97 ${CMAKE_CURRENT_BINARY_DIR}/test/fail40.json.h
98 ${CMAKE_CURRENT_BINARY_DIR}/test/fail41.json.h
99 ${CMAKE_CURRENT_BINARY_DIR}/test/fail42.json.h
100 ${CMAKE_CURRENT_BINARY_DIR}/test/fail44.json.h
101 ${CMAKE_CURRENT_BINARY_DIR}/test/fail45.json.h
102 ${CMAKE_CURRENT_BINARY_DIR}/test/pass1.json.h
103 ${CMAKE_CURRENT_BINARY_DIR}/test/pass2.json.h
104 ${CMAKE_CURRENT_BINARY_DIR}/test/pass3.json.h
105 ${CMAKE_CURRENT_BINARY_DIR}/test/pass4.json.h
106 ${CMAKE_CURRENT_BINARY_DIR}/test/round1.json.h
107 ${CMAKE_CURRENT_BINARY_DIR}/test/round2.json.h
108 ${CMAKE_CURRENT_BINARY_DIR}/test/round3.json.h
109 ${CMAKE_CURRENT_BINARY_DIR}/test/round4.json.h
110 ${CMAKE_CURRENT_BINARY_DIR}/test/round5.json.h
111 ${CMAKE_CURRENT_BINARY_DIR}/test/round6.json.h
112 ${CMAKE_CURRENT_BINARY_DIR}/test/round7.json.h
113 test/unitester.cpp
114 )
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.
95@@ -44,87 +96,64 @@ static void runtest(std::string filename, const std::string& jdata)
96 }
5@@ -6,7 +6,7 @@ cmake_path(GET JSON_SOURCE_PATH STEM json_source_basename)
6
7 file(READ ${JSON_SOURCE_PATH} hex_content HEX)
8 string(REGEX REPLACE "................" "\\0\n" formatted_bytes "${hex_content}")
9-string(REGEX REPLACE "[^\n][^\n]" "0x\\0, " formatted_bytes "${formatted_bytes}")
10+string(REGEX REPLACE "[^\n][^\n]" "char(0x\\0)," formatted_bytes "${formatted_bytes}")
we’re getting Functional-style cast is used instead of a C++ cast warning now, what if we used character literals instead:
0string(REGEX REPLACE "[^\n][^\n]" "'\\\\x\\0'," formatted_bytes "${formatted_bytes}")
which would look like:
0'\x5b','\x5c','\x6e','\x61','\x6b','\x65','\x64','\x5d',
instead of
0char(0x5b),char(0x5c),char(0x6e),char(0x61),char(0x6b),char(0x65),char(0x64),char(0x5d),
we’re getting
Functional-style cast is used instead of a C++ castwarning now, what if we used character literals instead:
Where? Functional-style casts are recommended by the dev notes, so if you want to change that, it would be good to change the dev notes first.
175- "round4.json", // bare number
176- "round5.json", // bare true
177- "round6.json", // bare false
178- "round7.json", // bare null
179-};
180+#define TEST_FILE(name) {#name, json_tests::name}
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:
0enum class TestType { Fail, Pass, RoundTrip };
and define the test as:
0inline constexpr std::map<std::string_view, TestType> tests{
1 {json_tests::fail1, TestType::Fail},
2 {json_tests::fail2, TestType::Fail},
3 {json_tests::fail3, TestType::Fail},
4...
which could simplify runtest to:
0static void runtest(std::string_view data, TestType type) {
1 UniValue val;
2 bool testResult = val.read(std::string{data});
3
4 if (type == TestType::Fail) {
5 assert(!testResult);
6 return;
7 }
8 ...
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 👍 We can continue the refactors in follow-up PRs.
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.
ACK faf7eac364fb7f421a649b483286ac8681d92b31
The changes since last ACK: simplify char casting and reformat the affected sources.