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): check for zebrad test output in the correct order #3643

Merged
merged 1 commit into from
Feb 25, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Feb 25, 2022

Motivation

The sync_until function was checking for mempool activation in unreliable and slow ways:

  1. The sync test was checking for mempool output after the stop log, but sometimes the mempool activates before the stop log
  2. The non-mempool tests were checking each individual log line, but they need to check the entire log

Solution

  1. The mempool is only activated once, so we must check for that log first. After mempool activation, the stop regex is always logged at least once. (It might be logged before mempool activation as well, but we can't rely on that.)

  2. When checking that the mempool didn't activate, wait for the zebrad command to exit, then check the entire log.

Review

@jvff can review this PR, but I'm going to cherry-pick it into #3582 as well.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

The mempool is only activated once, so we must check for that log first.
After mempool activation, the stop regex is logged at least once.
(It might be logged before as well, but we can't rely on that.)

When checking that the mempool didn't activate,
wait for the `zebrad` command to exit,
then check the entire log.
@teor2345 teor2345 added C-bug Category: This is a bug P-High 🔥 I-integration-fail Continuous integration fails, including build and test failures C-testing Category: These are tests labels Feb 25, 2022
@teor2345 teor2345 requested a review from jvff February 25, 2022 05:05
@teor2345 teor2345 self-assigned this Feb 25, 2022
@teor2345 teor2345 mentioned this pull request Feb 25, 2022
3 tasks
@codecov
Copy link

codecov bot commented Feb 25, 2022

Codecov Report

Merging #3643 (190cee2) into main (397ba1f) will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3643      +/-   ##
==========================================
- Coverage   79.95%   79.89%   -0.06%     
==========================================
  Files         281      281              
  Lines       32584    32584              
==========================================
- Hits        26052    26034      -18     
- Misses       6532     6550      +18     

Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Looks great! Simple and elegant solution 🎉

mergify bot added a commit that referenced this pull request Feb 25, 2022
@mergify mergify bot merged commit 729535c into main Feb 25, 2022
@mergify mergify bot deleted the full-sync-activation branch February 25, 2022 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug C-testing Category: These are tests I-integration-fail Continuous integration fails, including build and test failures
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants