Skip to content
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.

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

test: display abrupt shutdown errors in console output #28253

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

furszy
Copy link
Member

@furszy furszy commented Aug 10, 2023

Making it easier to debug errors in the CI environment,
particularly in scenarios where it's not immediately clear
what happened nor which node crashed (or shutdown abruptly).

A bit of context:
Currently, the test framework redirects each node's stderr output
stream to a different temporary file inside each node's data directory.
While this is sufficient for storing the error, it isn't very helpful for
understanding what happened just by reading the CI console output.

Most of the time, reading the stderr file in the CI environment is not
possible, because people don't have access to it.

Testing Note:
The displayed error difference can be observed by cherry-picking this
commit furszy@9cc5393 on top of this branch and running any
functional test.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 10, 2023

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK MarcoFalke, theStack
Concept ACK TheCharlatan
Stale ACK hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Tests label Aug 10, 2023
@TheCharlatan
Copy link
Contributor

Concept ACK

@maflcko
Copy link
Member

maflcko commented Aug 14, 2023

The CI uses the combine logs helper. Can you link to a CI output before and after?

@furszy
Copy link
Member Author

furszy commented Aug 14, 2023

The CI uses the combine logs helper. Can you link to a CI output before and after?

Sure.

Before https://github.com/furszy/bitcoin-core/runs/15887889073. No information provided.
After https://github.com/furszy/bitcoin-core/runs/15886179713. The abrupt shutdown reason is printed.

The commit causing the abrupt shutdown is the one shared in the PR description.

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

Ok, so this will only be hit when Bitcoin Core is completely non-functional and completely refuses to start (in create_cache.py)? In all other cases, this should already be printed.

test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
@furszy furszy force-pushed the 2023_test_framework_print_stderr branch 2 times, most recently from 2e6df14 to 6ac03a5 Compare August 23, 2023 14:24
Copy link
Member Author

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Updated per feedback. Thanks Marco.

Placed stderr error inside the thrown exception instead of printing it directly.
This is how the CI error looks now https://github.com/furszy/bitcoin-core/runs/16145060771.

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

lgtm ACK

test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
@furszy furszy force-pushed the 2023_test_framework_print_stderr branch from 6ac03a5 to 36b6f30 Compare August 23, 2023 15:17
@furszy
Copy link
Member Author

furszy commented Aug 23, 2023

Updated per feedback. Thanks.

Same CI result as before: https://github.com/furszy/bitcoin-core/runs/16146994349.

@hebasto
Copy link
Member

hebasto commented Oct 4, 2023

Concept ACK.

@theStack
Copy link
Contributor

theStack commented Oct 4, 2023

Concept ACK

@furszy furszy force-pushed the 2023_test_framework_print_stderr branch 2 times, most recently from 9923c59 to 2633152 Compare October 5, 2023 00:37
@furszy
Copy link
Member Author

furszy commented Oct 5, 2023

Updated per feedback, thanks theStack!

Diff is minimal.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 2633152, tested on GHA:

  • native macOS:
test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status -6 during initialization. 

************************
'node0' abruptly aborted with error:

EXCEPTION: St11logic_error       
I'm an ugly error. The blockchain is completely messed up.       
bitcoin in initload       

libc++abi: terminating due to uncaught exception of type std::logic_error: I'm an ugly error. The blockchain is completely messed up.
************************

2023-10-05T09:09:57.765000Z TestFramework (INFO): Stopping nodes
  • native Windows:
test_framework.test_node.FailedToStartError: [node 0] bitcoind exited with status 3 during initialization. 

************************
'node0' abruptly aborted with error:



EXCEPTION: class std::logic_error       

I'm an ugly error. The blockchain is completely messed up.       

D:\a\bitcoin\bitcoin\src\bitcoind.exe in initload       



************************

2023-10-05T09:10:35.169000Z TestFramework (INFO): Stopping nodes

@DrahtBot DrahtBot removed the CI failed label Oct 5, 2023
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

lgtm

test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

ACK 2633152

Making it easier to debug errors in the CI environment,
particularly in scenarios where it's not immediately clear
what happened nor which node crashed (or shutdown abruptly).
@furszy furszy force-pushed the 2023_test_framework_print_stderr branch from 2633152 to 0f83ab4 Compare October 5, 2023 12:46
@furszy
Copy link
Member Author

furszy commented Oct 5, 2023

Updated per feedback, small diff.
Now the code is much shorter.

@maflcko
Copy link
Member

maflcko commented Oct 5, 2023

lgtm ACK 0f83ab4

@DrahtBot DrahtBot requested a review from theStack October 5, 2023 12:50
@DrahtBot DrahtBot requested a review from hebasto October 5, 2023 12:50
@maflcko
Copy link
Member

maflcko commented Oct 5, 2023

Nice.

Just tested this to debug a crash and found it useful.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

ACK 0f83ab4

@maflcko maflcko added this to the 26.0 milestone Oct 6, 2023
@fanquake fanquake merged commit 1472df6 into bitcoin:master Oct 6, 2023
16 checks passed
@furszy furszy deleted the 2023_test_framework_print_stderr branch October 6, 2023 12:57
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants