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

fix(test): Show full Zebra test panic details in CI logs #4942

Merged
merged 11 commits into from
Aug 28, 2022
Merged

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Aug 24, 2022

Motivation

We're seeing some Zebra panics in CI. Our custom test harness just reports "The application panicked (crashed).", with no details.

Example logs:
https://github.com/ZcashFoundation/zebra/runs/7981879420?check_suite_focus=true#step:6:1103

Depends-On: #4945

Solution

  • Add output logs to test context, and add test harness tests to check it works
  • Let empty test child logs be read again (and produce empty output

Related refactors:

  • Add an ignore_exited argument to test child process kill()
  • Handle test failure regexes using Result::Err, rather than panicking

Related changes:

  • Log the command timeout when an acceptance test fails

Review

This is an urgent fix, so we can see panic details in our CI logs.

Reviewer Checklist

  • CI passes

@teor2345 teor2345 added C-bug Category: This is a bug P-Critical 🚑 A-diagnostics Area: Diagnosing issues or monitoring performance C-testing Category: These are tests labels Aug 24, 2022
@teor2345 teor2345 self-assigned this Aug 24, 2022
@teor2345 teor2345 requested a review from a team as a code owner August 24, 2022 03:24
@teor2345 teor2345 requested review from arya2 and removed request for a team August 24, 2022 03:24
@teor2345 teor2345 changed the title Show test panics Show full Zebra test panic details in CI logs Aug 24, 2022
@teor2345 teor2345 changed the title Show full Zebra test panic details in CI logs fix(test): Show full Zebra test panic details in CI logs Aug 24, 2022
@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Merging #4942 (1f72f3a) into main (1d861b0) will increase coverage by 0.06%.
The diff coverage is 76.69%.

@@            Coverage Diff             @@
##             main    #4942      +/-   ##
==========================================
+ Coverage   79.04%   79.11%   +0.06%     
==========================================
  Files         309      309              
  Lines       38781    38830      +49     
==========================================
+ Hits        30656    30720      +64     
+ Misses       8125     8110      -15     

@teor2345
Copy link
Contributor Author

Now we know what the panic bug is, this isn't a critical priority any more.

arya2
arya2 previously approved these changes Aug 24, 2022
zebra-test/src/command.rs Outdated Show resolved Hide resolved
zebra-test/src/command.rs Outdated Show resolved Hide resolved
zebrad/tests/common/sync.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 requested a review from a team as a code owner August 24, 2022 23:57
@teor2345 teor2345 changed the base branch from main to fix-ci-disk-space August 24, 2022 23:58
Base automatically changed from fix-ci-disk-space to main August 25, 2022 06:41
@teor2345
Copy link
Contributor Author

Marking this as critical because we're still seeing panics, and we need to know what they are. So we want to merge this first.

@teor2345
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Aug 25, 2022

update

✅ Branch has been successfully updated

@teor2345 teor2345 requested a review from arya2 August 25, 2022 22:00
@teor2345 teor2345 added the A-dependencies Area: Dependency file updates label Aug 26, 2022
@teor2345
Copy link
Contributor Author

I added some extra test debug output in commit 9d4c709. Alfredo and I talked about it earlier this week to make it easier to diagnose test failures.

I was going to put it in PR #4958, but that might have conflicted with this PR, so I put it here instead.

@teor2345
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Aug 28, 2022

update

✅ Branch has been successfully updated

Copy link
Member

@gustavovalverde gustavovalverde left a comment

Choose a reason for hiding this comment

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

Approving as this is critical and CI is passing.

mergify bot added a commit that referenced this pull request Aug 28, 2022
@mergify mergify bot merged commit f46d011 into main Aug 28, 2022
@mergify mergify bot deleted the show-test-panics branch August 28, 2022 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Dependency file updates A-diagnostics Area: Diagnosing issues or monitoring performance C-bug Category: This is a bug C-testing Category: These are tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants