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 timeout not being enforced by sync_until #4591

Closed
3 tasks
Tracked by #3096
conradoplg opened this issue Jun 10, 2022 · 4 comments
Closed
3 tasks
Tracked by #3096

Fix timeout not being enforced by sync_until #4591

conradoplg opened this issue Jun 10, 2022 · 4 comments
Labels
A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement C-testing Category: These are tests

Comments

@conradoplg
Copy link
Collaborator

conradoplg commented Jun 10, 2022

Motivation

The sync_until function, which is used by e.g. sync_one_checkpoint_testnet, does not enforce timeouts.

This makes hangs in the underlying issue harder to diagnose, since CI will only fail after the 6 hour timeout.

Make it always enforce the timeout.

Specifications

Designs

This is caused by wait_with_output being called when mempool activation is not required, and that function does not enforce timeouts (as documented).

Stop using wait_with_output:

  • Change sync_until to check the mempool didn't activate using with_failure_regex_iter()
  • Remove wait_with_output from sync_until, most of the checks are the same, only the mempool checks are different
  • After it's fixed, if possible, ensure that if the test fails, the captured output is printed to the terminal. This will make debugging issues much easier.

Related Work

@conradoplg conradoplg added A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage P-Medium ⚡ C-testing Category: These are tests labels Jun 10, 2022
@teor2345
Copy link
Contributor

Find out if wait_with_output needs to not enforce timeout, or if we just didn't add support at the time. Depending on the answer, make it enforce timeouts (one way to do that), or change sync_until to use another approach.

It's not possible to enforce timeouts using std::TestChild.

I've edited the ticket description to use with_failure_regex_iter - we didn't have that test harness feature when we added mempool support to sync_until.

We could also use process_control as a general fix for timeouts, and to simplify our code. But that seems like a bigger ticket.

@conradoplg
Copy link
Collaborator Author

I've edited the ticket description to use with_failure_regex_iter - we didn't have that test harness feature when we added mempool support to sync_until.

Thanks!

@ftm1000 ftm1000 removed the S-needs-triage Status: A bug report needs triage label Jun 23, 2022
@teor2345
Copy link
Contributor

This test bug does not block any of our release work.

@mpguerra mpguerra moved this to 🆕 New in Zebra Sep 22, 2022
@mpguerra mpguerra added this to Zebra Sep 22, 2022
@teor2345
Copy link
Contributor

This isn't actually causing us any problems at the moment

@teor2345 teor2345 closed this as not planned Won't fix, can't repro, duplicate, stale Oct 12, 2022
Repository owner moved this from 🆕 New to ✅ Done in Zebra Oct 12, 2022
@mpguerra mpguerra moved this from ✅ Done to 🛑 Won't Fix in Zebra Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement C-testing Category: These are tests
Projects
Archived in project
Development

No branches or pull requests

3 participants