Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: Create test utility library from src/test/util/ #17401

Closed
maflcko opened this issue Nov 7, 2019 · 7 comments
Closed

build: Create test utility library from src/test/util/ #17401

maflcko opened this issue Nov 7, 2019 · 7 comments

Comments

@maflcko
Copy link
Member

maflcko commented Nov 7, 2019

The purpose of the good first issue label is to highlight which issues are suitable for a new contributor without a deep understanding of the codebase.

Useful skills

  • Experience with autotools build systems

Task

The test utility library in https://github.com/bitcoin/bitcoin/tree/master/src/test/util is currently only a folder and each module in there is compiled for each target that uses it. For example, setup_common is compiled 27 times (for the fuzz suite) and another 3 times for the other tests (bench, unit test, gui). To speed up the compilation and clarify build dependencies, this folder should be transformed into an actual library.

Because our test binaries should test as much as possible, this library may depend on any Bitcoin Core library or third party library (except boost-unit-test). It should be built when at least one target that requires it is built, skipped otherwise.

Want to work on this issue?

You do not need to request permission to start working on this. You are encouraged to comment on the issue if you are planning to work on it. This will help other contributors monitor which issues are actively being addressed and is also an effective way to request assistance if and when you need it.

For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

@brakmic
Copy link
Contributor

brakmic commented Nov 20, 2019

Hi,

I've started working on this. Not sure if I understood the goals correctly (and if I should write anything here), but I can at least generate a static lib based on src/test/util sources. I've also removed the many src/util/test headers from Makefile.test.include, because I don't need them anymore to compile individual tests.
Instead, I've put into each test's LDADD the libtest_util.a, that is $(LIBTEST_UTIL). However, I'm not quite sure if this is the correct way to do it.

There's also a new Makefile.test_util.include.

Not sure if another Makefile would be allowed, but before I expand the already existing (and complex) ones, I think a small, separate Makefile is better.

Here the link to the commit in my forked repo: https://github.com/brakmic/bitcoin/commit/3f4b5e3a0b93c6b528bf707fd38eb63c0e29efa9

I'm working on macOS Catalina 10.15.1 with clang 9.0.0.

Regards,

@maflcko
Copy link
Member Author

maflcko commented Nov 20, 2019

Overall looks good, but the bench and gui tests are not using it?

@brakmic
Copy link
Contributor

brakmic commented Nov 20, 2019

Thanks!
I've included the missing references in bench & qttest.

Here's the commit: https://github.com/brakmic/bitcoin/commit/03002e3683c05dbd769efc7b2df206e0798bab3a

I've compiled bench/qttest and could execute them.

@brakmic
Copy link
Contributor

brakmic commented Nov 20, 2019

Oops, have forgotten to remove TEST_BITCOIN_H. Will do it now.

@brakmic
Copy link
Contributor

brakmic commented Nov 20, 2019

@maflcko
Copy link
Member Author

maflcko commented Nov 20, 2019

Ok, looks like you can open a pull request to get more review on it 👍

@brakmic
Copy link
Contributor

brakmic commented Nov 20, 2019

Many thanks. Will look into contribution guide first. ;)

maflcko pushed a commit to maflcko/bitcoin-core that referenced this issue Nov 22, 2019
…/util/

a2e581d build: Create test utility library from src/test/util/ (Harris)

Pull request description:

  This PR creates a static **test utility library** that replaces repetitive compilations of sources from *src/test/util* in **unit**, **gui** and **bench** **tests**.

  The original issue is here: bitcoin#17401

  The changes are:

  * a new *Makefile.test_util.include*
  * a new entry in *Makefile.am* that includes *Makefile.test_util.include* when testing is enabled
  * removal of all *src/test/util* headers & sources from unit, gui and bench Makefiles
  * addition of *libtest_util.a* at LDADD's of every test

ACKs for top commit:
  MarcoFalke:
    ACK a2e581d 🍞

Tree-SHA512: d172127a26ee70d16625e17d7d94337a65472c57bb97f910c357c52d3dc082ea478ee586ee9074d9ebfeb05b75027e5e15f5bcd2aa35962dadfd9ac6bfd55ab9
@maflcko maflcko closed this as completed Nov 22, 2019
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants