test: Move MakeNoLogFileContext to libtest_util, and use it in bench #21003

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2101-benchNoLog changing 15 files +38 −52

MarcoFalke commented at 10:43 AM on January 25, 2021: member

To avoid verbose code duplication, which may lead to accidental mishaps https://github.com/bitcoin/bitcoin/pull/20998/files#r563624041.

Also fix a nit I found in #20946 (review).
fanquake added the label Tests on Jan 25, 2021
MarcoFalke renamed this:
~~test: Move MakeNoLogFileContext to common libtest_util, and use it in bench~~
test: Move MakeNoLogFileContext to libtest_util, and use it in bench
on Jan 25, 2021
MarcoFalke closed this on Jan 25, 2021
MarcoFalke deleted the branch on Jan 25, 2021
MarcoFalke restored the branch on Jan 25, 2021
MarcoFalke commented at 7:17 PM on January 25, 2021: member

Worked around #21005
MarcoFalke reopened this on Jan 25, 2021
MarcoFalke force-pushed on Jan 25, 2021
DrahtBot commented at 2:15 AM on January 26, 2021: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #20773 (refactor: split CWallet::Create by S3RK)
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.
MarcoFalke force-pushed on Feb 11, 2021
DrahtBot added the label Needs rebase on Feb 11, 2021
MarcoFalke force-pushed on Feb 11, 2021
DrahtBot removed the label Needs rebase on Feb 11, 2021
DrahtBot added the label Needs rebase on Feb 11, 2021
MarcoFalke force-pushed on Feb 11, 2021
MarcoFalke commented at 2:06 PM on February 11, 2021: member

Rebased (3x)
DrahtBot removed the label Needs rebase on Feb 11, 2021
DrahtBot added the label Needs rebase on Feb 16, 2021
MarcoFalke force-pushed on Feb 17, 2021
fanquake commented at 10:24 AM on February 17, 2021: member

concept ack (1x)
DrahtBot removed the label Needs rebase on Feb 17, 2021
DrahtBot added the label Needs rebase on Feb 19, 2021
MarcoFalke force-pushed on Feb 19, 2021
MarcoFalke commented at 11:12 AM on February 19, 2021: member

Rebased (4x)
DrahtBot removed the label Needs rebase on Feb 19, 2021
DrahtBot added the label Needs rebase on Feb 20, 2021
MarcoFalke force-pushed on Feb 22, 2021
MarcoFalke commented at 9:25 AM on February 22, 2021: member

Rebased (5x)
DrahtBot removed the label Needs rebase on Feb 22, 2021
MarcoFalke referenced this in commit cac10e66d2 on Feb 25, 2021
MarcoFalke force-pushed on Feb 25, 2021
sidhujag referenced this in commit ea890fd13a on Feb 25, 2021
practicalswift commented at 4:03 PM on February 28, 2021: contributor

Concept ACK (2x)
DrahtBot added the label Needs rebase on Mar 1, 2021
MarcoFalke force-pushed on Mar 3, 2021
Move MakeNoLogFileContext to common libtest_util, and use it in bench
Can be reviewed with --color-moved=dimmed-zebra
fa576b4532
MarcoFalke force-pushed on Mar 3, 2021
DrahtBot removed the label Needs rebase on Mar 3, 2021

in src/bench/rpc_blockchain.cpp:18 in fa576b4532

  14 | @@ -15,7 +15,7 @@
  15 |  namespace {
  16 |  
  17 |  struct TestBlockAndIndex {
  18 | -    TestingSetup test_setup{};
  19 | +    const std::unique_ptr<const TestingSetup> testing_setup{MakeNoLogFileContext<const TestingSetup>(CBaseChainParams::MAIN)};

fanquake commented at 10:02 AM on March 3, 2021:

This seems kind of unruly.

MarcoFalke commented at 11:52 AM on March 3, 2021:

unruly

C++ requires this. (You can't use auto for non-static members)

fanquake approved
fanquake commented at 10:05 AM on March 3, 2021: member

ACK fa576b4532814b4bca1936d170cabd01fbc51960
MarcoFalke commented at 11:52 AM on March 3, 2021: member

@dongcarl Mind to take a look here, as this builds on top of #20946?
dongcarl commented at 7:23 PM on March 3, 2021: member

Light Code-Review ACK fa576b4532814b4bca1936d170cabd01fbc51960

Looks reasonable. Always good to coalesce these init codepaths.

fanquake commented at 12:50 AM on March 4, 2021: member

Failed merged (1x)

This need fixing up post #19203 & #19288:

test/fuzz/socks5.cpp:22:66: error: unknown type name 'BasicTestingSetup'
    static const auto testing_setup = MakeNoLogFileContext<const BasicTestingSetup>();
                                                                 ^
1 error generated.
gmake[2]: *** [Makefile:13693: test/fuzz/fuzz-socks5.o] Error 1
gmake[2]: *** Waiting for unfinished jobs....
test/fuzz/torcontrol.cpp:38:39: error: use of undeclared identifier 'MakeNoLogFileContext'
    static const auto testing_setup = MakeNoLogFileContext<>();
                                      ^
1 error generated.

diff --git a/src/test/fuzz/socks5.cpp b/src/test/fuzz/socks5.cpp
index 123ee042e..72c3e8f71 100644
--- a/src/test/fuzz/socks5.cpp
+++ b/src/test/fuzz/socks5.cpp
@@ -6,6 +6,7 @@
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
+#include <test/util/setup_common.h>
 
 #include <cstdint>
 #include <string>
diff --git a/src/test/fuzz/torcontrol.cpp b/src/test/fuzz/torcontrol.cpp
index b7a42ea7f..a8e69dee3 100644
--- a/src/test/fuzz/torcontrol.cpp
+++ b/src/test/fuzz/torcontrol.cpp
@@ -5,6 +5,7 @@
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
+#include <test/util/setup_common.h>
 #include <torcontrol.h>
 
 #include <cstdint>

MarcoFalke commented at 7:54 AM on March 4, 2021: member

Not going to invalidate the precious reviews over an unrelated issue. Fixed in #21358, which can go in first.
fanquake merged this on Mar 4, 2021
fanquake closed this on Mar 4, 2021
MarcoFalke deleted the branch on Mar 4, 2021
sidhujag referenced this in commit 0ac2c0e46a on Mar 4, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#20946 fuzz: Consolidate fuzzing TestingSetup initialization #20998 test: Fix BlockToJsonVerbose benchmark #21005 Remove gArgs #21264 fuzz: Two scripted diff renames

test: Move MakeNoLogFileContext to libtest_util, and use it in bench #21003

Conflicts