test: Move MakeNoLogFileContext to libtest_util, and use it in bench by maflcko · Pull Request #21003 · bitcoin/bitcoin

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


 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

Conversation

maflcko commented Jan 25, 2021

Uh oh!

maflcko commented Jan 25, 2021

Uh oh!

DrahtBot commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Feb 11, 2021

Uh oh!

maflcko commented Feb 19, 2021

Uh oh!

maflcko commented Feb 22, 2021

Uh oh!

practicalswift commented Feb 28, 2021

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 3, 2021

Uh oh!

dongcarl commented Mar 3, 2021

Uh oh!

fanquake commented Mar 4, 2021

Uh oh!

maflcko commented Mar 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DrahtBot commented Jan 26, 2021 •

edited

Loading