test, build: Separate `read_json` function into its own module #25974

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:220901-test changing 8 files +38 −21

hebasto commented at 4:20 PM on September 1, 2022: member

Currently, 4 source files rely on the definition of the read_json function provided in src/test/script_tests.cpp.

This PR breaks this entanglement, improves code structure and maintainability.
hebasto added the label Tests on Sep 1, 2022
in src/test/util/json.cpp:16 in 57c9bfabe6 outdated
```
  11 | +
  12 | +UniValue read_json(const std::string& jsondata)
  13 | +{
  14 | +    UniValue v;
  15 | +    if (!v.read(jsondata) || !v.isArray()) {
  16 | +        BOOST_ERROR("Parse error.");
```
maflcko commented at 10:38 AM on September 2, 2022:

In theory it is not allowed to link boosttest to libtest_util, but given that CI didn't fail, BOOST_ERROR is probably header-only?

hebasto commented at 11:01 AM on September 2, 2022:

39e66e938fb688f5400ad94a1b317fcc2a87bc31 from #24301 ?

maflcko commented at 11:33 AM on September 2, 2022:

Ah ok. I think I have a slight preference to keep boost out of libtest_util, and thus out of the fuzz tests

hebasto commented at 12:05 PM on September 2, 2022:

~What about just moving the read_json function into the src/test/main.cpp source file?~

hebasto commented at 12:08 PM on September 2, 2022:

Ah ok. I think I have a slight preference to keep boost out of libtest_util, and thus out of the fuzz tests

We already have: https://github.com/bitcoin/bitcoin/blob/ea67232cdb80c4bc3f16fcd823f6f811fd8903e1/src/test/util/chainstate.h#L17

fanquake commented at 12:11 PM on September 2, 2022:

We can probably remove that, and keep it Boost free.

maflcko commented at 12:46 PM on September 2, 2022:

Might be better to use Assert in any case to avoid the dead, confusing and dangerous return? (Dangerous because it returns an empty array, potentially running no tests at all)

maflcko commented at 12:49 PM on September 2, 2022:

We already have:

This can probably be replaced by a LogPrint or so

fanquake commented at 4:41 PM on September 5, 2022:

Done in #26009.

fanquake commented at 2:29 PM on September 13, 2022:

Now that #26009 has been merged, this should be reworked to avoid Boost usage.

hebasto commented at 3:58 PM on September 21, 2022:

Thanks! Updated.

hebasto commented at 3:58 PM on September 21, 2022:

Thanks! Updated.
theuni cross-referenced this on Sep 9, 2022 from issue test: remove Boost Test from libtest_util by fanquake
Malk8628 approved
hebasto force-pushed on Sep 21, 2022
hebasto commented at 3:58 PM on September 21, 2022: member
Updated 57c9bfabe6a1cfda2a179a457ab93d03b279ee30 -> aa326abcfc63d842e77feb5e3cffc19926369416 (pr25974.01 -> pr25974.02):
- addressed reviewers' comments
DrahtBot commented at 4:28 AM on September 23, 2022: contributor


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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #26940 (test: Move rand utils from setup_common to random and add a helper by jonatack)
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 cross-referenced this on Oct 10, 2022 from issue test: Remove unused txmempool include from tests by maflcko
DrahtBot added the label Needs rebase on Oct 19, 2022
hebasto force-pushed on Oct 20, 2022
hebasto commented at 12:19 PM on October 20, 2022: member

Rebased aa326abcfc63d842e77feb5e3cffc19926369416 -> 3394ad21b5a7e8ddfcc817e2eee996fb4b0bd4c6 (pr25974.02 -> pr25974.03) due to the conflict with #26286.
DrahtBot removed the label Needs rebase on Oct 20, 2022
DrahtBot cross-referenced this on Dec 22, 2022 from issue refactor: Avoid UniValue copies by maflcko
DrahtBot cross-referenced this on Jan 23, 2023 from issue test: create random and coins utils, add amount helper, dedupe add_coin by jonatack

Type	Reviewers
ACK	fanquake

in src/test/util/json.cpp:7 in 3394ad21b5 outdated

   0 | @@ -0,0 +1,18 @@
   1 | +// Copyright (c) 2022 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#include <test/util/json.h>
   6 | +
   7 | +#include <boost/test/unit_test.hpp>

maflcko commented at 1:04 PM on January 23, 2023:

why?

hebasto commented at 9:29 AM on January 27, 2023:

Thanks! Updated.

test, build: Separate `read_json` function into its own module 7a820cee0e
hebasto force-pushed on Jan 27, 2023
hebasto commented at 9:29 AM on January 27, 2023: member
Rebased 3394ad21b5a7e8ddfcc817e2eee996fb4b0bd4c6 -> 7a820cee0e6408f5848799011d82dd29ee7fa8c5 (pr25974.03 -> pr25974.04):
- addressed @MarcoFalke's comment
fanquake approved
fanquake commented at 3:07 PM on January 31, 2023: member

ACK 7a820cee0e6408f5848799011d82dd29ee7fa8c5
fanquake merged this on Feb 1, 2023
fanquake closed this on Feb 1, 2023
hebasto deleted the branch on Feb 1, 2023
sidhujag referenced this in commit 4fdb8db386 on Feb 1, 2023
bitcoin locked this on Feb 1, 2024

Contributors

Labels

Linked (view graph)

#24301 build: header-only Boost #26009 test: remove Boost Test from libtest_util

test, build: Separate `read_json` function into its own module #25974

Reviews

Conflicts