test: Split overly large util_tests.cpp file #26489

maflcko commented at 11:02 AM on November 11, 2022: member

The file has ~3kLOC and is slow to compile.

Fix both issues by splitting it. (On my machine the compilation goes from 25 seconds previously to 17+10 seconds for the two smaller files)

To review, --color-moved=dimmed-zebra can be used.

DrahtBot added the label Tests on Nov 11, 2022

DrahtBot commented at 11:37 PM on November 11, 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:

#26213 (univalue: Remove confusing getBool/isTrue/isFalse by MarcoFalke)
#26028 (More verbose warning for multiple network argument error. by amovfx)
#17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)
#16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

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 Nov 12, 2022 from issue rpc: Strict type checking for RPC boolean parameters by maflcko

in src/test/argsman_tests.cpp:17 in fae2420f16 outdated

  12 | +#include <univalue.h>
  13 | +
  14 | +#include <array>
  15 | +#include <optional>
  16 | +#include <cstdint>
  17 | +#include <cstring>

shaavan commented at 8:05 AM on November 12, 2022:

nit: These dependencies can be removed

maflcko commented at 8:14 AM on November 12, 2022:

No, they are needed, see https://en.cppreference.com/w/cpp/types/integer

shaavan commented at 1:34 PM on November 14, 2022:

Okay, I verified that two dependencies are indeed needed for the file.

maflcko commented at 1:37 PM on November 14, 2022:

I hope we can use iwyu one day on the tests :sweat_smile:

shaavan commented at 1:40 PM on November 14, 2022:

Thanks a lot for the name. I was searching for this tool, but I forgot its name. I will remember to use it from now on. 😄

shaavan commented at 8:11 AM on November 12, 2022: contributor

Concept ACK

I successfully compiled the branch and ran the test on it.

I think some dependencies would be redundant and can be removed.

in src/test/util_tests.cpp

#include <test/util/logging.h>
#include <test/util/str.h>

and, in src/test/argsman_tests.cpp

DrahtBot cross-referenced this on Nov 12, 2022 from issue More verbose warning for multiple network argument error. by amovfx

DrahtBot cross-referenced this on Nov 14, 2022 from issue util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky

DrahtBot cross-referenced this on Nov 14, 2022 from issue refactor: Remove settings merge reverse precedence code by ryanofsky

DrahtBot cross-referenced this on Nov 14, 2022 from issue refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky

DrahtBot cross-referenced this on Nov 14, 2022 from issue util: Forbid ambiguous multiple assignments in config file by ryanofsky

DrahtBot cross-referenced this on Nov 14, 2022 from issue refactor: Implement missing error checking for ArgsManager flags by ryanofsky

test: Split overly large util_tests.cpp file fa4ec1be51

maflcko force-pushed on Nov 14, 2022

shaavan approved

shaavan commented at 1:37 PM on November 14, 2022: contributor

ACK fa4ec1be513c80d6db644f1e3170e980035e7306

The redundant dependencies are removed from the src/test/util_tests.cpp file.

in src/test/argsman_tests.cpp:5 in fa4ec1be51

   0 | @@ -0,0 +1,1043 @@
   1 | +// Copyright (c) 2011-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 <util/system.h>

aureleoules commented at 2:42 PM on November 15, 2022:

nit

diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp
index d00876bc7..76d64e0d5 100644
--- a/src/test/argsman_tests.cpp
+++ b/src/test/argsman_tests.cpp
@@ -2,13 +2,13 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <util/system.h>
 #include <fs.h>
 #include <sync.h>
 #include <test/util/logging.h>
 #include <test/util/setup_common.h>
 #include <test/util/str.h>
 #include <util/strencodings.h>
+#include <util/system.h>
 #include <univalue.h>
 
 #include <array>

aureleoules approved

aureleoules commented at 2:43 PM on November 15, 2022: member

ACK fa4ec1be513c80d6db644f1e3170e980035e7306

RandyMcMillan commented at 4:25 PM on November 15, 2022: contributor

ACK fa4ec1be513c80d6db644f1e3170e980035e7306 for:

macOS x84_64: Darwin ₿ 19.6.0 Darwin Kernel Version 19.6.0: Tue Jun 21 21:18:39 PDT 2022; root:xnu-6153.141.66~1/RELEASE_X86_64 x86_64

macOS Arm64: Darwin DeepSpaceM1.local 21.6.0 Darwin Kernel Version 21.6.0: Thu Sep 29 20:13:46 PDT 2022; root:xnu-8020.240.7~1/RELEASE_ARM64_T8101 arm64

maflcko merged this on Nov 15, 2022

maflcko closed this on Nov 15, 2022

maflcko deleted the branch on Nov 15, 2022

sidhujag referenced this in commit 068335a15c on Nov 16, 2022

bitcoin locked this on Nov 15, 2023