refactor: univalue test cleanups #25617

fanquake commented at 9:20 AM on July 15, 2022: member

Remove references to "downstream" from makefiles, as they are now redundant. Remove BOOST_TEST macros in favour of just using functions. Add missing call to univalue_push_throw tests.

fanquake added the label Refactoring on Jul 15, 2022

in src/univalue/test/test_json.cpp:2 in 54e5b6c61a outdated

   0 | @@ -1,26 +0,0 @@
   1 | -// Test program that can be called by the JSON test suite at
   2 | -// https://github.com/nst/JSONTestSuite.

MarcoFalke commented at 9:49 AM on July 15, 2022:

Not sure if we should remove this. Developers can still use it locally or it can be added to CI later?

fanquake commented at 10:00 AM on July 15, 2022:

I doubt any developer is ever going to use this locally. It could be added to the CI (and looks like it at least partly was without realising this code is never compiled), but I'm not sure it's worth it.

in src/univalue/test/object.cpp:18 in 49d600a274 outdated

  12 | @@ -13,11 +13,6 @@
  13 |  #include <string>
  14 |  #include <vector>
  15 |  
  16 | -#define BOOST_FIXTURE_TEST_SUITE(a, b)
  17 | -#define BOOST_AUTO_TEST_CASE(funcName) void funcName()
  18 | -#define BOOST_AUTO_TEST_SUITE_END()
  19 | -#define BOOST_CHECK(expr) assert(expr)
  20 | -#define BOOST_CHECK_EQUAL(v1, v2) assert((v1) == (v2))
  21 |  #define BOOST_CHECK_THROW(stmt, excMatch) { \

MarcoFalke commented at 9:53 AM on July 15, 2022:

This just seems to inline some checks while leaving others as-is. I can't really see a motivation to do this, especially as this is causing needless merge conflicts and inconsistent code.

I am fine removing the empty TEST macros, as they are unused. However, the other macros are used, so it would be better to keep them.

fanquake commented at 10:00 AM on July 15, 2022:

while leaving others as-is.

The others will be removed later. I'm planning on further consolidating the test code. I've just PR'd these changes first.

MarcoFalke commented at 9:53 AM on July 15, 2022: member

idk

DrahtBot commented at 10:26 AM on July 15, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23670 (build: Build test binaries in make check, not in make by hebasto)

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.

fanquake force-pushed on Jul 15, 2022

fanquake commented at 3:44 PM on July 15, 2022: member

Rebased and simplified to the non-controversial changes.

fanquake force-pushed on Jul 15, 2022

in src/univalue/test/unitester.cpp:162 in 004e20a527 outdated

 156 | @@ -157,6 +157,8 @@ int main (int argc, char *argv[])
 157 |  
 158 |      unescape_unicode_test();
 159 |  
 160 | -    return 0;
 161 | +    char buf[] = "___[1,2,3]___";
 162 | +    UniValue val;
 163 | +    return val.read(buf + 3, 7) ? 0 : 1;

MarcoFalke commented at 3:51 PM on July 15, 2022:

nit: Could use assert like in the rest of this file?

assert(val.read(buf + 3, 7))

fanquake commented at 9:23 AM on July 17, 2022:

Done

fanquake force-pushed on Jul 17, 2022

in src/univalue/test/unitester.cpp:162 in 9c12d72322 outdated

 156 | @@ -157,6 +157,9 @@ int main (int argc, char *argv[])
 157 |  
 158 |      unescape_unicode_test();
 159 |  
 160 | +    char buf[] = "___[1,2,3]___";
 161 | +    UniValue val;
 162 | +    assert(val.read(buf + 3, 7));

MarcoFalke commented at 7:31 AM on July 18, 2022:

It might be good to move this into a function, otherwise it will be hard to understand the purpose of the test from looking at the code.

fanquake commented at 8:16 AM on July 18, 2022:

Moved into a function.

doc: remove references to downstream

Having references to downstream no-longer make sense now that we've
unsubtree'd.

98a0ae6b24

refactor: integrate no_nul into univalue unitester 70d807c355

refactor: remove BOOST_*_TEST_ macros 1f0c83f430

fanquake force-pushed on Jul 18, 2022

MarcoFalke commented at 8:29 AM on July 18, 2022: member

Good catch on the test issue. Also, good cleanup.

ACK 1f0c83f43092f6bc959bcb1036a7076cb1235309 🍎

<details><summary>Show signature</summary>

Signature:

</details>

MarcoFalke merged this on Jul 18, 2022

MarcoFalke closed this on Jul 18, 2022

fanquake deleted the branch on Jul 18, 2022

sidhujag referenced this in commit 088584ace1 on Jul 18, 2022

bitcoin locked this on Jul 18, 2023