test: Create new test library #17384
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
I'd be interested to know what the motivation for this is, are you planning to use this library from multiple binaries, or is it just organizational? |
Yes, that is the motivation that I mentioned in the readme. Also, we compile each file in the library (as of today) multiple times (for each target), so that should be fixed as well. |
Can you give an example? |
setup_common is compiled 27 times (for the fuzz suite) and another 3 times for the other tests (bench, unit test, gui) |
|
How about moving the content of |
Whoa. Yes, then it's quite a serious issue! |
Good point. I think I will leave this for later, so that this changeset can be kept a move-only scripted-diff |
|
This pull should also fix the appveyor failures: #17357 (comment) |
|
I've cancelled all appveyor builds, since they are about to fail anyway. Let's see if this one passes. |
src/test/lib/README.md
Outdated
| Generally, the files in this folder should be well separated modules. New code should be added to existing modules or | ||
| (when in doubt) a new module should be created. | ||
|
|
||
| The utilities in here are compiled into a library, which does not hold any state. However, the main file `setup_common` |
There was a problem hiding this comment.
nit: can remove the comma after "library"
There was a problem hiding this comment.
You were right. A non-restrictive clause beginning with "that" would take no comma, but a non-restrictive clause beginning with "which" does take a comma.
|
appveyor passed: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/28656027 |
|
Added a dot at the end of the sentence, among other minor doc fixups as requested by @jonatack No code changes, so the previous ci builds are still valid. |
There was a problem hiding this comment.
Code review ACK fa6fb57. Seems good to separate test utilities from unit tests, since it's natural for things like benchmarks, fuzzing, or simulation tools to depend on test utilities, but you wouldn't want those things depending on unit tests.
Feel free to ignore this suggestion, but I think it'd be better to rename src/test/lib directory back to src/test/util. It's normal to see lib prefixes in library paths (especially since they get added automatically by the linker), but not really normal to see lib/ in #include strings, and not something we have for any of our other libraries.
-BEGIN VERIFY SCRIPT- # Move files for f in $(git ls-files src/test/lib/); do git mv $f src/test/util/; done git mv src/test/setup_common.cpp src/test/util/ git mv src/test/setup_common.h src/test/util/ # Replace Windows paths sed -i -e 's|\\setup_common|\\util\\setup_common|g' $(git grep -l '\\setup_common') sed -i -e 's|src\\test\\lib\\|src\\test\\util\\|g' build_msvc/test_bitcoin/test_bitcoin.vcxproj # Everything else sed -i -e 's|/setup_common|/util/setup_common|g' $(git grep -l 'setup_common') sed -i -e 's|test/lib/|test/util/|g' $(git grep -l 'test/lib/') # Fix include guard sed -i -e 's|BITCOIN_TEST_SETUP_COMMON_H|BITCOIN_TEST_UTIL_SETUP_COMMON_H|g' ./src/test/util/setup_common.h sed -i -e 's|BITCOIN_TEST_LIB_|BITCOIN_TEST_UTIL_|g' $(git grep -l 'BITCOIN_TEST_LIB_') -END VERIFY SCRIPT-
|
Renamed to test/util/ as requested by @ryanofsky |
|
Both travis and appveyor passes (so this fixes the appveyor build). Anything left to do here? |
|
ACK fa4c6fa -- diff looks correct and Travis is happy |
This hasn't had a ton of review, but it seems fairly safe to merge as it only affects test code. |
|
I agree. I am concept ACK on both organizing all shared functions in one place, and on eventually turning this into a library. The code changes look good to me, but I didn't test. Just noting that the added documentation in |
And are those always compiled with the same flags etc? |
|
Scripted diff ACK. |
|
ACK fa4c6fa with the reserve that the commit messages (and PR description) contain the motivation for this change. Built, ran tests, light code review. |
|
Added motivation to OP as requested by @jonatack |
fa4c6fa doc: Add documentation for new test/lib (MarcoFalke) faec282 scripted-diff: test: Move setup_common to test library (MarcoFalke) Pull request description: Sorry for clickbait, this is only a move-only scripted-diff commit and one documentation commit. Longer term, someone who knows something about build systems can make this an actual library. Motivation for this is that each module gets compiled for each target that includes 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) ACKs for top commit: practicalswift: ACK fa4c6fa -- diff looks correct and Travis is happy jonatack: ACK fa4c6fa with the reserve that the commit messages (and PR description) contain the motivation for this change. Built, ran tests, light code review. ryanofsky: Code review ACK fa4c6fa. I didn't realize `lib` was actually name of existing directory, not a new name. But in any case this looks good and nice to have one scripted diff instead of two. Tree-SHA512: 2e176df90c60578276e4a6dc83ff57ff59d8e666ecf30c5ceacb8c326725da91baa4cac3dfa7a2e1605f58122a3e3e27e4938ff33e3a0ce7ea53afffebbf57a4
fa4c6fa doc: Add documentation for new test/lib (MarcoFalke) faec282 scripted-diff: test: Move setup_common to test library (MarcoFalke) Pull request description: Sorry for clickbait, this is only a move-only scripted-diff commit and one documentation commit. Longer term, someone who knows something about build systems can make this an actual library. Motivation for this is that each module gets compiled for each target that includes 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) ACKs for top commit: practicalswift: ACK fa4c6fa -- diff looks correct and Travis is happy jonatack: ACK fa4c6fa with the reserve that the commit messages (and PR description) contain the motivation for this change. Built, ran tests, light code review. ryanofsky: Code review ACK fa4c6fa. I didn't realize `lib` was actually name of existing directory, not a new name. But in any case this looks good and nice to have one scripted diff instead of two. Tree-SHA512: 2e176df90c60578276e4a6dc83ff57ff59d8e666ecf30c5ceacb8c326725da91baa4cac3dfa7a2e1605f58122a3e3e27e4938ff33e3a0ce7ea53afffebbf57a4
…svc project b80f7db Remove redundant class file includes from test_bitcoin project. (Aaron Clauson) Pull request description: #17364 & #17384 overlapped and both added the same line of `..\..\src\test\util\*.cpp` to `test_bitcoin.vcxproj`. This didn't break the build but does result in duplicate symbol warnings. This PR cleans it up and removes the additional redundant line of `..\..\src\test\util\setup_common.cpp` which will also be covered by the wildcard include. ACKs for top commit: MarcoFalke: ACK b80f7db 🔅 fanquake: ACK b80f7db - tested a build on a Windows machine. No longer see the warnings shown below: Tree-SHA512: 55960821480483c517b475f2a6871cd7d4033d086db3fd679aa0de362e4f7e2c3ac7967ca278cc3728cc765ba23d4441ec769d83d7a47e7a3fa2f09de2bbc145
Summary: * scripted-diff: test: Move setup_common to test library -BEGIN VERIFY SCRIPT- # Move files for f in $(git ls-files src/test/lib/); do git mv $f src/test/util/; done git mv src/test/setup_common.cpp src/test/util/ git mv src/test/setup_common.h src/test/util/ # Replace Windows paths sed -i -e 's|\\setup_common|\\util\\setup_common|g' $(git grep -l '\\setup_common') sed -i -e 's|src\\test\\lib\\|src\\test\\util\\|g' build_msvc/test_bitcoin/test_bitcoin.vcxproj # Everything else sed -i -e 's|/setup_common|/util/setup_common|g' $(git grep -l 'setup_common') sed -i -e 's|test/lib/|test/util/|g' $(git grep -l 'test/lib/') # Fix include guard sed -i -e 's|BITCOIN_TEST_SETUP_COMMON_H|BITCOIN_TEST_UTIL_SETUP_COMMON_H|g' ./src/test/util/setup_common.h sed -i -e 's|BITCOIN_TEST_LIB_|BITCOIN_TEST_UTIL_|g' $(git grep -l 'BITCOIN_TEST_LIB_') -END VERIFY SCRIPT- * doc: Add documentation for new test/lib This is a backport of Core [[bitcoin/bitcoin#17384 | PR17384]] Depends on D6358 Test Plan: ninaj all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6360
fa4c6fa doc: Add documentation for new test/lib (MarcoFalke) faec282 scripted-diff: test: Move setup_common to test library (MarcoFalke) Pull request description: Sorry for clickbait, this is only a move-only scripted-diff commit and one documentation commit. Longer term, someone who knows something about build systems can make this an actual library. Motivation for this is that each module gets compiled for each target that includes 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) ACKs for top commit: practicalswift: ACK fa4c6fa -- diff looks correct and Travis is happy jonatack: ACK fa4c6fa with the reserve that the commit messages (and PR description) contain the motivation for this change. Built, ran tests, light code review. ryanofsky: Code review ACK fa4c6fa. I didn't realize `lib` was actually name of existing directory, not a new name. But in any case this looks good and nice to have one scripted diff instead of two. Tree-SHA512: 2e176df90c60578276e4a6dc83ff57ff59d8e666ecf30c5ceacb8c326725da91baa4cac3dfa7a2e1605f58122a3e3e27e4938ff33e3a0ce7ea53afffebbf57a4
Sorry for clickbait, this is only a move-only scripted-diff commit and one documentation commit.
Longer term, someone who knows something about build systems can make this an actual library. Motivation for this is that each module gets compiled for each target that includes 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)