unit tests can be really slow under wine because BOOST_CHECK logs something for all tests. This patch makes them faster by only logging tests which fail. PR’d an alternative to #8632.
real 3m52.840s user 3m52.217s sys 0m0.241s
real 0m30.358s user 0m29.695s sys 0m0.165s
real 0m20.445s user 0m19.888s sys 0m0.193s
real 0m20.887s user 0m20.360s sys 0m0.108s
real 0m11.894s user 0m11.645s sys 0m0.060s
Sounds impressive.
Here are the important definitions (this PRs diff): https://github.com/bitcoin/bitcoin/pull/8650/files#diff-4a0cb5ad5e93d187a1b065a99bbae143R17
Documentation: http://www.boost.org/doc/libs/1_34_0/libs/test/doc/components/test_tools/reference/BOOST_CHECK_MESSAGE.html
Concept ACK
Concept ACK. After:
0git diff HEAD~1 --name-only | xargs sed -i "s/FAST_CHECK/BOOST_CHECK/g"
There is a remaining diff in test_bitcoin.h:
0+#define BOOST_CHECK(x) if (!(x)) { BOOST_CHECK_MESSAGE(false, #x); }
1+#define BOOST_CHECK_EQUAL(x, y) if ((x)!=(y)) { BOOST_CHECK_MESSAGE(false, "(" #x ") != (" #y ")"); }
2+#define BOOST_CHECK_THROW(expr, ex) try { expr; BOOST_CHECK_MESSAGE(false, "( " #expr " ) did not throw ( " #ex " )"); } catch( ex ) { }
3+#define BOOST_CHECK_NO_THROW(expr) try { expr; } catch( ... ) {BOOST_CHECK_MESSAGE(false, "( " #expr " ) threw exception"); }
4+#define BOOST_CHECK_EXCEPTION(expr, ex, pred) try { expr; BOOST_CHECK_MESSAGE(false, "( " #expr " ) did not throw ( " #ex " )"); } catch( ex& a ) { if (!pred(a)) BOOST_CHECK_MESSAGE(false, "( " #expr " ) did not throw ( " #ex " ) under ( " #pred " )" ); }
5+#define BOOST_CHECK_EQUAL_COLLECTIONS(l, r, l1, r1) BOOST_CHECK_EQUAL_COLLECTIONS(l, r, l1, r1)
6+
Any specific reason for (re-)defining these?
Oh never mind - these are replaced of course @jonasschnelli has the right definitions. So you define custom testing macros, right.
Q: What functionality do we lose here, apart from being able to log successful tests (which is arguably unnecessary)? For example I vaguely remember that the BOOST_CHECK_EQUAL does an attempt to print the values, not just the stringified form of the input arguments.
At some point we likely want to get rid of boost::test completely. as we are trying to eventually remove the dependency on boost, so this is a step in the right direction.
@laanwj I may be able to make it completely compatible minus the succeeding tests for each macro. You are correct that boost_check_equal does that; this is part of why I did not redefine the container equal one.
What I’m unsure of is how that affects licensing given those would be a derivative work I believe – perhaps I could break out my macros into a sub-library and keep it boost licensed?
perhaps I could break out my macros into a sub-library and keep it boost licensed?
Right, that makes sense: if you’re using any boost code then it’s a derivative work, if you’re just using the names of the macros for compatibility it’s not. Do note that the boost license is more permissive than MIT/BSD (no attribution requirement), so including a boost-licensed source file is not a problem.
Huh? I didn’t see any boost source code added, so the current pull should be good license wise.
Fast check is just a wrapper around boost check, which should be fine as long as you don’t distribute fast check as a separate tool.
Huh? I didn’t see any boost source code added, so the current pull should be good license wise.
Yes the current pull is, but (read back a few posts) he is talking about extending the functionality to something completely compatible.
@laanwj I don’t see how we lose functionality.
@ Jeremy Check your mail. It should be trivial to keep the log messages the same as well
On Friday, September 2, 2016, Wladimir J. van der Laan < notifications@github.com> wrote:
Q: What functionality do we lose here, apart from being able to log successful tests (which is arguably unnecessary)? For example I vaguely remember that the BOOST_CHECK_EQUAL does an attempt to print the values, not just the stringified form of the input arguments.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.< https://ci5.googleusercontent.com/proxy/y-3C4A3Q1wrYbmH2Ylkt2A0H5t_g18JH93W1j4X-rMvxva57umGu8LYbGWrx-2KG43iGXvTJzOYsJCFPwGAeUUoshU8yjvuwI0DUJmPFos2gj90HbuEBbw1ji_Lzy2RjVD8YeGwUXfgyHCv6q1DB9eGtazfHQQ=s0-d-e1-ft#https://github.com/notifications/beacon/AGGmv0llHp5MBym3vyMXyqBWvwnwX5n9ks5ql-RjgaJpZM4JzXok.gif>
@laanwj I don’t see how we lose functionality.
That was my question. One thing I noticed was that BOOST_EQUAL no longer prints the contents of what it compares but just stringified variable names (which is much less useful). But I was not sure either if there is more.
A few notes:
I think that 4. is probably the best option longer term.
Concept ACK.
As far as lost functionality, note that we also lose the checkpoints this way, so if a segfault is introduced, it’s possible that it’s harder to tell where it’s coming from. I suspect that’s part of what makes the macros so expensive to begin with.
I don’t think those are a huge loss, they’ve never helped me much.
Ok, here’s what I’m going to do. I’ve just opened up #8671 which does a very minimal change to prevector tests that should fix the underlying issue; while giving more useful debugging output (the rand seeds needed to recreate this test case). I think there’s no reason this can’t merge almost immediately as a much needed stopgap to make the tests just run faster.
In the meantime, I’ve started working on a boost-free test suite and opened an issue about it #8670. All necessary features should go in there, such as checkpoints being needed by @sipa.
(@MarcoFalke @laanwj @theuni @jonasschnelli )
