test: Show debug log on unit test failure #16975
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fa758c2 to
fa8e20a
Compare
|
Can be tested by injecting a failure in any test case and then running |
|
Concept ACK. Ran diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index fed65afdbf..d0948dcbca 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -88,6 +88,7 @@ BOOST_AUTO_TEST_CASE(cnode_listen_port)
BOOST_CHECK(gArgs.SoftSetArg("-port", std::to_string(altPort)));
port = GetListenPort();
BOOST_CHECK(port == altPort);
+ BOOST_CHECK(false);
}
BOOST_AUTO_TEST_CASE(caddrdb_read) |
|
Whoops. Fixing ... |
faaa2d3 to
fa006d8
Compare
|
Fixed |
jamesob
reviewed
Sep 27, 2019
src/logging.h
Outdated
| @@ -77,6 +77,9 @@ namespace BCLog { | |||
|
|
|||
| std::string LogTimestampStr(const std::string& str); | |||
|
|
|||
| /** Slots that connect to the print signal */ | |||
| std::list<std::function<void(const std::string&)>> m_print_callbacks /* GUARDED_BY(m_cs) */ {}; | |||
There was a problem hiding this comment.
Why comment the lock annotation?
There was a problem hiding this comment.
It can't compile otherwise. If you include sync.h, the tests won't run after compilation.
src/logging.h
Outdated
| } | ||
|
|
||
| /** Delete a connection */ | ||
| void DeleteCallback(std::list<std::function<void(const std::string&)>>::iterator it) |
64fac9a to
fa39655
Compare
|
Concept ACK |
|
Concept ACK. |
|
Concept ACK. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
fa116ab to
faa5a01
Compare
|
Ah, forgot to push one more tsan suppression. Fixed. Though, longer term I'd like to do #8670 (comment) |
|
utACK faa5a01. Tried a few different scenarios and noted that logging is provided for multiple failed test cases in a file before exiting. For example: |
faa5a01 to
fa031ab
Compare
|
Rebased for minor merge conflict |
|
Concept ACK If there are multiple test cases in a single test file and one of them fails currently the logs of all the test cases are printed, also the succeeding ones. Could that be improved? It could be especially annoying if there are a lot of logs for every test case and the developer would have to search for the failing one (I discovered this on |
|
@fjahr I don't think I can solve that without having a way to detect (inside the Also, I think having too much logs is better than having no logs to debug at all. |
fa031ab to
fa9ab18
Compare
|
Concept ACK.
Usually true, though having too much (unrelated) logging can also distract. And CI web interfaces make the experience of browsing large files quite horrible. Also, too much logging can slow down the tests. But I don't think that's the case with this change. |
fa55a17 to
fab72a3
Compare
Agree on the distraction part, but if you set the log level to "debug" or "test_suite" I think a verbose log should be expected. Note that this is not printed to the ci log unless the test fails. Also note that the tests already log to the debug.log, so logging to the test log shouldn't slow them down. See the comparison: So it might or might not be slower, but I don't think there is any significant difference |
fab72a3 to
fad5fe2
Compare
promag
reviewed
Nov 24, 2019
|
Anything left to do here? |
|
ACK fad5fe2 -- diff looks correct |
fad5fe2 to
fa37e0a
Compare
|
Rebased to fix trivial merge conflict |
|
Would-be ACK fa37e0a but it looks like I've hit a deadlock. Modified the coins test for failure: diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 436c1bffa0..2c20efc12b 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -213,6 +213,7 @@ BOOST_AUTO_TEST_CASE(coins_cache_simulation_test)
unsigned int flushIndex = InsecureRandRange(stack.size() - 1);
BOOST_CHECK(stack[flushIndex]->Flush());
}
+ assert(false);
}
if (InsecureRandRange(100) == 0) {
// Every 100 iterations, change the cache stack.and ran with Noticed the test seemed to be hanging, so straced: |
|
ACK fa37e0a ( Confirmed that Show signature data
|
laanwj
added a commit
that referenced
this pull request
Jan 8, 2020
fa37e0a test: Show debug log on unit test failure (MarcoFalke) Pull request description: Often, it is hard to debug unit test failures without the debug log. Especially when the failure happens remotely (e.g. on a ci system). Fix that by printing the log on failure. ACKs for top commit: jamesob: ACK fa37e0a ([`jamesob/ackr/16975.1.MarcoFalke.test_show_debug_log_on_u`](https://github.com/jamesob/bitcoin/tree/ackr/16975.1.MarcoFalke.test_show_debug_log_on_u)) Tree-SHA512: 2ca4150c4ae3d4ad47e03b5e5e70da2baffec928ddef1fdf53a3ebc061f14aee249205387cb1b12ef6d4eb55711ef0080c0b41d9d18000b5da124ca80299793b
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jan 8, 2020
fa37e0a test: Show debug log on unit test failure (MarcoFalke) Pull request description: Often, it is hard to debug unit test failures without the debug log. Especially when the failure happens remotely (e.g. on a ci system). Fix that by printing the log on failure. ACKs for top commit: jamesob: ACK fa37e0a ([`jamesob/ackr/16975.1.MarcoFalke.test_show_debug_log_on_u`](https://github.com/jamesob/bitcoin/tree/ackr/16975.1.MarcoFalke.test_show_debug_log_on_u)) Tree-SHA512: 2ca4150c4ae3d4ad47e03b5e5e70da2baffec928ddef1fdf53a3ebc061f14aee249205387cb1b12ef6d4eb55711ef0080c0b41d9d18000b5da124ca80299793b
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
Apr 1, 2020
7777703 doc: Explain new test logging (MarcoFalke) Pull request description: Explain logging added in bitcoin#18472 and bitcoin#16975 ACKs for top commit: jonatack: ACK 7777703 Tree-SHA512: 3a0aa7bab32a6753d8894d29cf82604b044b23e512102dd275b717eefda3c2212dbf43ea7e9155267350dd9f3bc5badba2eb660152db3efeab30a04f52126c95
sidhujag
pushed a commit
to syscoin-core/syscoin
that referenced
this pull request
Nov 10, 2020
fa37e0a test: Show debug log on unit test failure (MarcoFalke) Pull request description: Often, it is hard to debug unit test failures without the debug log. Especially when the failure happens remotely (e.g. on a ci system). Fix that by printing the log on failure. ACKs for top commit: jamesob: ACK fa37e0a ([`jamesob/ackr/16975.1.MarcoFalke.test_show_debug_log_on_u`](https://github.com/jamesob/bitcoin/tree/ackr/16975.1.MarcoFalke.test_show_debug_log_on_u)) Tree-SHA512: 2ca4150c4ae3d4ad47e03b5e5e70da2baffec928ddef1fdf53a3ebc061f14aee249205387cb1b12ef6d4eb55711ef0080c0b41d9d18000b5da124ca80299793b
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Aug 31, 2021
Summary: This is a backport of [[bitcoin/bitcoin#16975 | core#16975]] Test Plan: `ninja check` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9971
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Often, it is hard to debug unit test failures without the debug log. Especially when the failure happens remotely (e.g. on a ci system).
Fix that by printing the log on failure.