Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Oct 10, 2022

Seems odd to include this heavy header in all tests despite it only being used in a few tests.

Can be reviewed with --color-moved=dimmed-zebra --ignore-all-space

@fanquake fanquake added the Tests label Oct 10, 2022
@maflcko
Copy link
Member Author

maflcko commented Oct 10, 2022

Follow-up to #26250

@fanquake
Copy link
Member

Concept ACK - looks like some previously transitively included includes will need to be added:

test/util_tests.cpp: In member function ‘void util_tests::util_seed_insecure_rand::test_method()’:
test/util_tests.cpp:1441:33: error: ‘sqrt’ was not declared in this scope
 1441 |         int err = 30*10000./mod*sqrt((1./mod*(1-1./mod))/10000.);
      |                                 ^~~~
make[2]: *** [Makefile:18499: test/test_bitcoin-util_tests.o] Error 1

@hebasto
Copy link
Member

hebasto commented Oct 10, 2022

Concept ACK.

@maflcko maflcko force-pushed the 2210-test-cleanup- branch from fa407d1 to fae702d Compare October 10, 2022 13:11
@maflcko
Copy link
Member Author

maflcko commented Oct 10, 2022

I can reduce the fuzz compile time (user+kernel time) by 20% with a dirty txmempool.h. To reproduce:

( echo >> ./src/txmempool.h ) && /usr/bin/time --format '%U+%S (%e)' make -j $(nproc)

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 10, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26153 (Reduce wasted pseudorandom bytes in ChaCha20 + various improvements by sipa)
  • #25977 (refactor: Replace std::optional<bilingual_str> with util::Result by ryanofsky)
  • #25974 (test, build: Separate read_json function into its own module by hebasto)
  • #25909 (wallet: coverage for receiving txes with same id but different witness data by furszy)
  • #25908 (p2p: remove adjusted time by fanquake)
  • #25361 (BIP324: Cipher suite by dhruv)
  • #25325 (Add pool based memory resource by martinus)
  • #25172 (refactor: use std:: prefix for std lib funcs by fanquake)
  • #25152 (refactor: Split util/system into exception, shell, and fs-specific files by Empact)
  • #23561 (BIP324: Handshake prerequisites by dhruv)
  • #10443 (Add fee_est tool for debugging fee estimation code 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.

@brunoerg
Copy link
Contributor

Concept ACK

@maflcko
Copy link
Member Author

maflcko commented Oct 11, 2022

Does anyone know why msvc is unable to link this?

@fanquake
Copy link
Member

Does anyone know why msvc is unable to link this?

@sipsorcery @hebasto any thoughts?

@hebasto
Copy link
Member

hebasto commented Oct 12, 2022

Does anyone know why msvc is unable to link this?

--- a/build_msvc/test_bitcoin-qt/test_bitcoin-qt.vcxproj
+++ b/build_msvc/test_bitcoin-qt/test_bitcoin-qt.vcxproj
@@ -54,6 +54,9 @@
     <ProjectReference Include="..\libbitcoin_zmq\libbitcoin_zmq.vcxproj">
       <Project>{792d487f-f14c-49fc-a9de-3fc150f31c3f}</Project>
     </ProjectReference>
+    <ProjectReference Include="..\libtest_util\libtest_util.vcxproj">
+      <Project>{1e065f03-3566-47d0-8fa9-daa72b084e7d}</Project>
+    </ProjectReference>
     <ProjectReference Include="..\libleveldb\libleveldb.vcxproj">
       <Project>{18430fef-6b61-4c53-b396-718e02850f1b}</Project>
     </ProjectReference>

See https://cirrus-ci.com/task/6132886477733888.

Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fad7f22

Co-authored-by: Aurèle Oulès <aurele@oules.com>
Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK 1c48dae
Thanks for the co-author.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1c48dae, I have reviewed the code and it looks OK, I agree it can be merged.

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1c48dae

@maflcko maflcko merged commit 003050d into bitcoin:master Oct 19, 2022
@maflcko maflcko deleted the 2210-test-cleanup-🐼 branch October 19, 2022 08:16
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 23, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Oct 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants