[Merged by Bors] - Allow TaskExecutor to be used in async tests
#3178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduce harness to test harness Thread task_executor into BeaconChain
paulhauner
changed the title
TaskExecutor to be used in async tests
paulhauner
added
ready-for-review
AgeManning
reviewed
May 16, 2022
| @@ -61,6 +61,7 @@ execution_layer = { path = "../execution_layer" } | |||
| sensitive_url = { path = "../../common/sensitive_url" } | |||
| superstruct = "0.5.0" | |||
| hex = "0.4.2" | |||
| exit-future = "0.2.0" | |||
There was a problem hiding this comment.
We use him in beacon_chain::test_utils.
There was a problem hiding this comment.
If this is just testing? Can it be a dev-dep then?
There was a problem hiding this comment.
It's just for testing, but it's not hidden behind a #[test] compilation flag so it must be a major dependency. We can't hide it behind a #[test] flag since it's used across multiple crates and Cargo doesn't play nicely with that.
macladson
approved these changes
May 16, 2022
Co-authored-by: Mac L <[email protected]>
|
Thanks for the reviews! bors r+ |
paulhauner
added
ready-for-merge
bors bot
pushed a commit
that referenced
this pull request
May 16, 2022
# Description Since the `TaskExecutor` currently requires a `Weak<Runtime>`, it's impossible to use it in an async test where the `Runtime` is created outside our scope. Whilst we *could* create a new `Runtime` instance inside the async test, dropping that `Runtime` would cause a panic (you can't drop a `Runtime` in an async context). To address this issue, this PR creates the `enum Handle`, which supports either: - A `Weak<Runtime>` (for use in our production code) - A `Handle` to a runtime (for use in testing) In theory, there should be no change to the behaviour of our production code (beyond some slightly different descriptions in HTTP 500 errors), or even our tests. If there is no change, you might ask *"why bother?"*. There are two PRs (#3070 and #3175) that are waiting on these fixes to introduce some new tests. Since we've added the EL to the `BeaconChain` (for the merge), we are now doing more async stuff in tests. I've also added a `RuntimeExecutor` to the `BeaconChainTestHarness`. Whilst that's not immediately useful, it will become useful in the near future with all the new async testing.
bors
bot
changed the title
TaskExecutor to be used in async testsTaskExecutor to be used in async tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Since the
TaskExecutorcurrently requires aWeak<Runtime>, it's impossible to use it in an async test where theRuntimeis created outside our scope. Whilst we could create a newRuntimeinstance inside the async test, dropping thatRuntimewould cause a panic (you can't drop aRuntimein an async context).To address this issue, this PR creates the
enum Handle, which supports either:Weak<Runtime>(for use in our production code)Handleto a runtime (for use in testing)In theory, there should be no change to the behaviour of our production code (beyond some slightly different descriptions in HTTP 500 errors), or even our tests. If there is no change, you might ask "why bother?". There are two PRs (#3070 and #3175) that are waiting on these fixes to introduce some new tests. Since we've added the EL to the
BeaconChain(for the merge), we are now doing more async stuff in tests.I've also added a
RuntimeExecutorto theBeaconChainTestHarness. Whilst that's not immediately useful, it will become useful in the near future with all the new async testing.