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

Create a shared Tokio runtime for tests #2397

Merged
merged 2 commits into from
Jul 1, 2021

Conversation

jvff
Copy link
Contributor

@jvff jvff commented Jun 25, 2021

Motivation

Some Zebra tests had to be grouped to be executed in a single test, in order to share the Tokio runtime. The reason for this is detailed in #2390.

Solution

This draft PR was opened to test an idea for a new solution, that's currently not described in the original issue (#2390). The idea is to create a zebra_test::RUNTIME lazily initialized global variable with a Tokio runtime. This runtime can then be reused by the tests, without having to be grouped into a single test.

Review

Reviewer Checklist

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

Follow Up Work

Controlling time

One issue with the current PR is that the tests that use the runtime and pause the internal Tokio timer must resume it before it finishes, or risk affecting other tests that use the shared runtime.

Some helper code could be written to help with this. However, there is no easy solution, because there's no easy way to check if time is frozen or not, and because pausing or resuming panics if the timer is in the incorrect state.

One idea is to pause time with a guard type that resumes the timer when it is dropped.

Another idea might be to always start the tests with a pause followed by a resume call, in order to fail fast if some test fails to resume the timer.

Improving ergonomics

Since the call to zebra_test::init() and the zebra_test::RUNTIME.block_on must be called for every test, it might make sense to create a zebra-test-macros crate with a #[zebra_test::test] macro that can call zebra_test::init() automatically before the test, and call zebra_test::RUNTIME.block_on if the test function is async.

@zfnd-bot zfnd-bot bot assigned jvff Jun 25, 2021
@jvff jvff requested a review from teor2345 June 25, 2021 23:52
@teor2345
Copy link
Contributor

One issue with the current PR is that the tests that use the runtime and pause the internal Tokio timer must resume it before it finishes, or risk affecting other tests that use the shared runtime.

I'm not sure if this condition is enough for correct test results. What if multiple tests are running concurrently?

(By default, Rust runs each test in a separate thread. I'm not sure what tokio::test does.)

@teor2345
Copy link
Contributor

@jvff is this PR blocking any work for the NU5 testnet release?
(We're trying to keep scope as small as possible to reduce work, and limit changes to reduce risk.)

@jvff
Copy link
Contributor Author

jvff commented Jun 28, 2021

Oh, right. Concurrency might complicate things. It might be simpler to just avoid time related tests when using the shared runtime, and using the shared runtime only where it's necessary to avoid the batch verifier issue.

It's not actually blocking any work, but it does make writing tests that use batch verifiers easier. Otherwise there's extra work to figure out if a test will work, running the tests on both my computer and the CI (because it's not always that the test fails on an environment due to the issue), and in some cases redesigning tests so that they can be grouped to form one larger test to share the runtime. This redesign usually makes tests harder to reason and more error-prone :/

With this PR, I can write the tests that I know use a batch verifier without worrying about false negatives caused by the shared runtime. This is a patch solution until a longer term solution is implemented (we can keep track of that in #2390).

@teor2345 teor2345 added A-dependencies Area: Dependency file updates A-rust Area: Updates to Rust code C-bug Category: This is a bug P-Medium and removed P-Optional labels Jun 30, 2021
@teor2345 teor2345 added this to the 2021 Sprint 13 milestone Jun 30, 2021
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

It's really important that we document when to use the shared runtime, and when to avoid it.

Otherwise this PR is good and we should merge it.

zebra-test/src/lib.rs Outdated Show resolved Hide resolved
jvff added 2 commits June 30, 2021 18:46
Create a lazily instantiated Tokio runtime that can be shared by tests.
Split two tests that were previously in one because of the need to share
a single Tokio runtime. With the `zebra_test::RUNTIME`, they can now
share the runtime without having to be a single test.
@jvff jvff force-pushed the shared-tokio-runtime-for-tests branch from 3250c1e to c8609a9 Compare June 30, 2021 18:46
@jvff jvff marked this pull request as ready for review June 30, 2021 19:16
@jvff jvff requested a review from teor2345 June 30, 2021 19:16
Copy link
Contributor

@teor2345 teor2345 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 let's go

@teor2345 teor2345 merged commit f33923f into ZcashFoundation:main Jul 1, 2021
@jvff jvff deleted the shared-tokio-runtime-for-tests branch July 1, 2021 01:01
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-rust Area: Updates to Rust code C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix tasks spawned by tower_batch::Batch only being alive during one test execution
2 participants