Skip to content

test: Add ASSERT_DEBUG_LOG to unit test framework #16540

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 2 commits into from
Nov 5, 2019
Merged

test: Add ASSERT_DEBUG_LOG to unit test framework #16540

maflcko merged 2 commits into from
Nov 5, 2019

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Aug 2, 2019 •
edited
Loading

Similar to assert_debug_log in the functional test framework

@maflcko maflcko force-pushed the 1908-UnitTestAssertDebugLog branch from fa79298 to fa3cd66 Compare August 2, 2019 20:43
@fanquake fanquake added the Tests label Aug 2, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 2, 2019 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16224 (gui: Bilingual GUI error messages by hebasto)

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.

@maflcko maflcko force-pushed the 1908-UnitTestAssertDebugLog branch 4 times, most recently from fa6aeeb to fae9929 Compare August 3, 2019 14:18
promag
promag reviewed Aug 6, 2019
View reviewed changes
practicalswift
practicalswift reviewed Aug 16, 2019
View reviewed changes
@maflcko maflcko force-pushed the 1908-UnitTestAssertDebugLog branch 2 times, most recently from 3333dee to fa98bd1 Compare August 20, 2019 20:10
@practicalswift
Copy link
Contributor

practicalswift commented Aug 20, 2019

Concept ACK

@maflcko maflcko force-pushed the 1908-UnitTestAssertDebugLog branch 3 times, most recently from fac57b4 to fad4319 Compare September 27, 2019 19:20
@maflcko maflcko mentioned this pull request Sep 27, 2019
Merged
@maflcko
Copy link
Member Author

maflcko commented Oct 3, 2019

re-run ci

@maflcko maflcko closed this Oct 3, 2019
@maflcko maflcko reopened this Oct 3, 2019
@ajtowns
Copy link
Contributor

ajtowns commented Oct 4, 2019

Concept ACK. I think this might be better if implemented with a RAII approach, so you write:

{
    ASSERT_DEBUG_LOG("xyzzy");
    code();
}

and an error appears if the string wasn't found by the end of the block. See https://github.com/ajtowns/bitcoin/commits/201910-assertdebuglog for a patch. This way avoids the globals and lets you just do two asserts in the same block rather than having to always create a vector. I also changed it so it only captures the log if you say ASSERT_DEBUG_LOG_CAPTURE("blah");.

@maflcko maflcko force-pushed the 1908-UnitTestAssertDebugLog branch from fad4319 to faa78a6 Compare October 8, 2019 13:45
@maflcko
Copy link
Member Author

maflcko commented Oct 8, 2019

Thanks, taken some of the patch and rebased

@maflcko maflcko force-pushed the 1908-UnitTestAssertDebugLog branch from faa78a6 to fafa1b9 Compare October 23, 2019 13:46
@ajtowns
Copy link
Contributor

ajtowns commented Oct 25, 2019

If I change ASSERT_DEBUG_LOG("is not a directory") to "isnt a directory" to trigger an error, it seems to hang after the failure rather than finishing shutdown?

@maflcko
Copy link
Member Author

maflcko commented Oct 30, 2019

@ajtowns This is an issue every time an assert is hit. The tests in that case would sleep forever, due to some boost internals, see #16700.

The correct error is still printed and the tests fail. I am not sure how to fix this minor usability issue, given that the logger is global (shared state) and the TestingSetup is not shut down between test cases. (make check shuts down the TestingSetup between test suites, so that is something at least)

@maflcko maflcko force-pushed the 1908-UnitTestAssertDebugLog branch from fafa1b9 to fa2c44c Compare November 4, 2019 15:42
@maflcko
Copy link
Member Author

maflcko commented Nov 4, 2019

Rebased

@ajtowns
Copy link
Contributor

ajtowns commented Nov 4, 2019

ACK fafa1b931ad9a69922bac53fb0a49d568719c2c2

@maflcko
Copy link
Member Author

maflcko commented Nov 4, 2019

@laanwj Any objections? Otherwise I will merge tomorrow

maflcko pushed a commit that referenced this pull request Nov 5, 2019
Merge #16540: test: Add ASSERT_DEBUG_LOG to unit test framework
fea532a 
fa2c44c test: Add ASSERT_DEBUG_LOG to unit test framework (MarcoFalke)
fa1936f logging: Add member for arbitrary print callbacks (MarcoFalke)

Pull request description:

  Similar to `assert_debug_log` in the functional test framework

Top commit has no ACKs.

Tree-SHA512: aa9eaeca386b61d806867c04a33275f6eb4624fa5bf50f2928d16c83f5634bac96bcac46f9e8eda3b00b4251c5f12d7b01d6ffd84ba8e05c09eeec810cc31251
@maflcko maflcko merged commit fa2c44c into bitcoin:master Nov 5, 2019
@maflcko maflcko deleted the 1908-UnitTestAssertDebugLog branch November 5, 2019 19:37
ryanofsky
ryanofsky reviewed Nov 5, 2019
View reviewed changes
src/test/lib/logging.cpp
noui_reconnect();
LogInstance().DeleteCallback(m_print_connection);
if (!m_found) {
throw std::runtime_error(strprintf("'%s' not found in debug log\n", m_message));
Copy link
Contributor

@ryanofsky ryanofsky Nov 5, 2019

Choose a reason for hiding this comment

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

Haven't looked closely, but I'm kind of surprised you can get away with throwing an exception in a destructor (check_found is called by ~DebugLogHelper).

If this works, it seems fine. But in case there are issues, an alternative could be to use a callback instead of a scope, so instead of:

{
    ASSERT_DEBUG_LOG("Please check that your computer's date and time are correct!");
    MultiAddTimeData(1, DEFAULT_MAX_TIME_ADJUSTMENT + 1); //filter size 5
}

Something like:

AssertDebugLog("Please check that your computer's date and time are correct!", [&] {
    MultiAddTimeData(1, DEFAULT_MAX_TIME_ADJUSTMENT + 1); //filter size 5
});

Copy link
Contributor

@ajtowns ajtowns Nov 6, 2019

Choose a reason for hiding this comment

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

I believe C++11 makes destructors implicitly noexcept, which means that an uncaught exception from results in std::terminate() being called, which is what we want from throwing a runtime exception anyway

Copy link
Member Author

@maflcko maflcko Nov 7, 2019

Choose a reason for hiding this comment

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

I forgot about this, but indeed the test seem to fail either way, which is what we want.

@hebasto hebasto mentioned this pull request Nov 6, 2019
Merged
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Apr 30, 2020
@deadalnix
test: Add ASSERT_DEBUG_LOG to unit test framework
4bd613e 
Summary:
 * logging: Add member for arbitrary print callbacks

 * test: Add ASSERT_DEBUG_LOG to unit test framework

This is a backport of Core [[bitcoin/bitcoin#16540 | PR16540]]

Test Plan:
  make check

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5909
@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.

Reviewers

@ryanofsky ryanofsky ryanofsky left review comments

+3 more reviewers

@ajtowns ajtowns ajtowns left review comments

@promag promag promag left review comments

@practicalswift practicalswift practicalswift left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@maflcko @DrahtBot @practicalswift @ajtowns @promag @ryanofsky @fanquake