[Tests] Suppress output in test_bitcoin for expected errors #16277

Closes #15944

This adds two methods to noui, that allows temporarily suppressing (and then resuming) the output from noui. For situations where errors are expected, it's confusing for the test binary to output an error and then conclude with No errors detected.

It also uses this supress/reconnect in the tests that currently produce verbose errors when running test_bitcoin.

Output of test_bitcoin on current master:

gertjaap@gjdesktop:~/src/bitcoin$ src/test/test_bitcoin
Running 351 test cases...
Error: Specified -walletdir "/tmp/test_common_Bitcoin Core/1561389554_943311758/tempdir/path_does_not_exist" does not exist
Error: Specified -walletdir "/tmp/test_common_Bitcoin Core/1561389554_643733972/tempdir/not_a_directory.dat" is not a directory
Error: Specified -walletdir "wallets" is a relative path

*** No errors detected

Output after this code is merged:

gertjaap@gjdesktop:~/src/bitcoin$ src/test/test_bitcoin
Running 351 test cases...

*** No errors detected

Concept ACK

Thanks for working on this: those expected errors are really annoying.

Nice first-time contribution! Welcome! :-)

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

Thanks for tackling this, this has been bothering me for a while.

Addressed review comments and squashed

ACK 6d7bcf9, reviewed code and tested his (also checked that reconnect works by suppressing only the first of the three loud tests). Nice to get rid of these error messages, this might also encourage more negative unit tests that people might have shied away from in fear of spamming stderr even more.

Addressed review comment & squashed

So a future change after this would be to make it possible to catch the output, and retrieve it for checking, as well as suppressing it. I think that could be done in a future PR and do think this making the test output not contain unexpected errors is an improvement, as-is.

Suppressing the output is one way to solve the issue.

However, I don't think that we are addressing the fundamental issue, which is that VerifyWallets doesn't propagate error information to the caller. Instead VerifyWallets keeps that information to itself.

Functions like VerifyWallets, which don't provide error information to the caller and seem to use logging of the error message as a substitute, should ideally return an expected success or error object to the caller to allow it to handle recoverable errors.

With a Rust-like Result<T, E> type we can resolve #15944 as follows:

// VerifyWallet returns an error object of type std::string.
Result<void, std::string> result = VerifyWallets(...);

// The caller handles recoverable errors in the way it deems appropriate.
if (error result) {
    log error;
    return;
}

I dropped WIP code here https://github.com/bitcoin/bitcoin/compare/master...l2a5b1:feature/result-type?expand=1 that shows how we can handle recoverable errors with a Result<T,E> type as an alternative to returning a bool and hiding error information from the caller.

This code also fixes #15944 but it does this by way of propagating the error information to the caller, allowing the caller to log the error message or to ignore it in the tests.

If suppressing output is however preferred then this PR seems like a nice way to do it!

I do like the idea of a Result type. Are there any C++ libraries that use or offer (or plan to offer) such a type? I.e boost or the standard library.

This sounds like it would benefit from a project-wide discussion. Maybe open a brainstorming issue or raise the topic in one of the meetings.

Yess, I too prefer rust's method of error handling, but I think asking a first-time contributor to rewrite the project's general error-handling methodology is a bit wild, please take that into account here.

Already working on fix for that error type scenario may not be exactly the way rust method. I think it will have the same effect will see🤞better then a rewrite anyway.

On Thu, Jul 11, 2019, 1:23 PM Wladimir J. van der Laan < notifications@github.com> wrote:

Yess, I too prefer rust's method of error handling, but I think asking a first-time contributor to rewrite the project's general error-handling methodology is a bit wild, please take that into account here.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/16277?email_source=notifications&email_token=AKW4MAX5G66YRWKIPKJCISDP653AJA5CNFSM4H27XUYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZXRXEY#issuecomment-510598035, or mute the thread https://github.com/notifications/unsubscribe-auth/AKW4MAV3Q4B6G63HEADGSTTP653AJANCNFSM4H27XUYA .

I like the suggestion @l2a5b1 made, of a Result<T, E> return value to functions that now return bool and don't propagate error info. I think the project would benefit a lot from something like that. That said, it seems like something we should discuss in broader perspective as @MarcoFalke suggested. I'd be happy to contribute to that discussion and/or help implementing the preferred solution.

I enjoy thinking about larger refactoring as much as the next guy, and if this PR triggers a broader discussion that ends up improving Bitcoin's error-handling, then I'll take that as a win. I do however think that this broader discussion might take time and effort, where this PR provides an - in my view - acceptable solution to the problem it's addressing today, and is solving something people are actually annoyed by for some time.

ACK 7a0c224 - tested and reviewed.

Thanks @gertjaap! I will take the suggestion pertaining to propagating errors somewhere else.

ACK 7a0c2242890549b6dddac4cc6f1840a528469f2a