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

maflcko · 2021-01-25T10:43:44Z

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 (comment).

maflcko · 2021-01-25T19:17:59Z

Worked around #21005

DrahtBot · 2021-01-26T02:15:16Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: split CWallet::Create #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.

maflcko · 2021-02-11T14:06:12Z

Rebased (3x)

maflcko · 2021-02-19T11:12:24Z

Rebased (4x)

maflcko · 2021-02-22T09:25:38Z

Rebased (5x)

fae216a scripted-diff: Rename MakeFuzzingContext to MakeNoLogFileContext (MarcoFalke) fa4fbec scripted-diff: Rename PROVIDE_MAIN_FUNCTION -> PROVIDE_FUZZ_MAIN_FUNCTION (MarcoFalke) Pull request description: Split out two renames from #21003: * `PROVIDE_FUZZ_MAIN_FUNCTION`. *Reason*: This in only used by fuzzing, so the name should indicate that. * `MakeNoLogFileContext`. *Reason*: Better reflects what the helper does. Also, prepares it to be used in non-fuzz tests in the future. ACKs for top commit: practicalswift: cr ACK fae216a: scripted-diff looks correct Tree-SHA512: e5d347746f5da72b0c86fd4f07ac2e4b3016e88e8c97a830c73bd79d0af6d0245fe7712487fc20344d6cc25958941716c1678124a123930407e3a437265b71df

fae216a scripted-diff: Rename MakeFuzzingContext to MakeNoLogFileContext (MarcoFalke) fa4fbec scripted-diff: Rename PROVIDE_MAIN_FUNCTION -> PROVIDE_FUZZ_MAIN_FUNCTION (MarcoFalke) Pull request description: Split out two renames from bitcoin#21003: * `PROVIDE_FUZZ_MAIN_FUNCTION`. *Reason*: This in only used by fuzzing, so the name should indicate that. * `MakeNoLogFileContext`. *Reason*: Better reflects what the helper does. Also, prepares it to be used in non-fuzz tests in the future. ACKs for top commit: practicalswift: cr ACK fae216a: scripted-diff looks correct Tree-SHA512: e5d347746f5da72b0c86fd4f07ac2e4b3016e88e8c97a830c73bd79d0af6d0245fe7712487fc20344d6cc25958941716c1678124a123930407e3a437265b71df

practicalswift · 2021-02-28T16:03:49Z

Concept ACK (2x)

Can be reviewed with --color-moved=dimmed-zebra

fanquake

ACK fa576b4

fanquake · 2021-03-03T10:02:14Z

src/bench/rpc_blockchain.cpp

@@ -15,7 +15,7 @@
 namespace {

 struct TestBlockAndIndex {
-    TestingSetup test_setup{};
+    const std::unique_ptr<const TestingSetup> testing_setup{MakeNoLogFileContext<const TestingSetup>(CBaseChainParams::MAIN)};


This seems kind of unruly.

unruly

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

maflcko · 2021-03-03T11:52:56Z

@dongcarl Mind to take a look here, as this builds on top of #20946?

dongcarl · 2021-03-03T19:23:24Z

Light Code-Review ACK fa576b4

Looks reasonable. Always good to coalesce these init codepaths.

fanquake · 2021-03-04T00:50:16Z

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>

maflcko · 2021-03-04T07:54:07Z

Not going to invalidate the precious reviews over an unrelated issue. Fixed in #21358, which can go in first.

… and use it in bench fa576b4 Move MakeNoLogFileContext to common libtest_util, and use it in bench (MarcoFalke) Pull request description: 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 bitcoin#20946 (comment). ACKs for top commit: dongcarl: Light Code-Review ACK fa576b4 fanquake: ACK fa576b4 Tree-SHA512: d39ac9c0957813ebb20ed13bd25a8ef8469377ce2651245638bd761c796feac2a17e30dd16f1e5c8db57737fb918c81d56a3d784c33258275e426a1b11e69eb2

fanquake added the Tests label Jan 25, 2021

This was referenced Jan 25, 2021

fuzz: Consolidate fuzzing TestingSetup initialization #20946

Merged

test: Fix BlockToJsonVerbose benchmark #20998

Merged

maflcko changed the title ~~test: Move MakeNoLogFileContext to common libtest_util, and use it in bench~~ test: Move MakeNoLogFileContext to libtest_util, and use it in bench Jan 25, 2021

maflcko closed this Jan 25, 2021

maflcko deleted the 2101-benchNoLog branch January 25, 2021 11:33

maflcko mentioned this pull request Jan 25, 2021

Remove gArgs #21005

Closed

maflcko restored the 2101-benchNoLog branch January 25, 2021 19:17

maflcko reopened this Jan 25, 2021

maflcko force-pushed the 2101-benchNoLog branch from fa6770f to fa26edc Compare January 25, 2021 19:18

DrahtBot mentioned this pull request Feb 2, 2021

refactor: return MempoolAcceptResult from ATMP #21062

Merged

This was referenced Feb 10, 2021

fuzz: Add tx_pool fuzz target #21142

Merged

validation/coins: limit MemPoolAccept access to coins cache + make it responsible for uncaching #21146

Closed

maflcko force-pushed the 2101-benchNoLog branch from fa26edc to fa15266 Compare February 11, 2021 08:55

DrahtBot added the Needs rebase label Feb 11, 2021

maflcko force-pushed the 2101-benchNoLog branch from fa15266 to fa5ebb4 Compare February 11, 2021 10:07

DrahtBot added Needs rebase and removed Needs rebase labels Feb 11, 2021

maflcko force-pushed the 2101-benchNoLog branch from fa5ebb4 to ccccc62 Compare February 11, 2021 14:05

DrahtBot removed the Needs rebase label Feb 11, 2021

DrahtBot removed the Needs rebase label Feb 17, 2021

DrahtBot mentioned this pull request Feb 19, 2021

build: Fix fuzz binary compilation under windows #21226

Merged

DrahtBot added the Needs rebase label Feb 19, 2021

maflcko force-pushed the 2101-benchNoLog branch from fa6aed8 to fa5e65e Compare February 19, 2021 11:11

DrahtBot added Needs rebase and removed Needs rebase labels Feb 19, 2021

maflcko force-pushed the 2101-benchNoLog branch from fa5e65e to fa2b92a Compare February 22, 2021 08:59

maflcko mentioned this pull request Feb 22, 2021

fuzz: Two scripted diff renames #21264

Merged

DrahtBot removed the Needs rebase label Feb 22, 2021

maflcko force-pushed the 2101-benchNoLog branch from fa2b92a to fa9640c Compare February 25, 2021 13:48

DrahtBot added the Needs rebase label Mar 1, 2021

maflcko force-pushed the 2101-benchNoLog branch from fa9640c to fa684aa Compare March 3, 2021 08:13

Move MakeNoLogFileContext to common libtest_util, and use it in bench

fa576b4

Can be reviewed with --color-moved=dimmed-zebra

maflcko force-pushed the 2101-benchNoLog branch from fa684aa to fa576b4 Compare March 3, 2021 08:23

DrahtBot removed the Needs rebase label Mar 3, 2021

fanquake approved these changes Mar 3, 2021

View reviewed changes

maflcko mentioned this pull request Mar 4, 2021

fuzz: Add missing include (test/util/setup_common.h) #21358

Merged

fanquake merged commit 83bdbbd into bitcoin:master Mar 4, 2021

maflcko deleted the 2101-benchNoLog branch March 4, 2021 15:32

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

maflcko commented Jan 25, 2021

maflcko commented Jan 25, 2021

DrahtBot commented Jan 26, 2021 •

edited

Loading

maflcko commented Feb 11, 2021

maflcko commented Feb 19, 2021

maflcko commented Feb 22, 2021

practicalswift commented Feb 28, 2021

fanquake left a comment

fanquake Mar 3, 2021

maflcko Mar 3, 2021

maflcko commented Mar 3, 2021

dongcarl commented Mar 3, 2021

fanquake commented Mar 4, 2021

maflcko commented Mar 4, 2021

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

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

Conversation

maflcko commented Jan 25, 2021

maflcko commented Jan 25, 2021

DrahtBot commented Jan 26, 2021 • edited Loading

Conflicts

maflcko commented Feb 11, 2021

maflcko commented Feb 19, 2021

maflcko commented Feb 22, 2021

practicalswift commented Feb 28, 2021

fanquake left a comment

Choose a reason for hiding this comment

fanquake Mar 3, 2021

Choose a reason for hiding this comment

maflcko Mar 3, 2021

Choose a reason for hiding this comment

maflcko commented Mar 3, 2021

dongcarl commented Mar 3, 2021

fanquake commented Mar 4, 2021

maflcko commented Mar 4, 2021

DrahtBot commented Jan 26, 2021 •

edited

Loading