This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Add test suite and minor refinements to the utility subsystem#1403
Merged
Add test suite and minor refinements to the utility subsystem#1403
Conversation
Unfortunately, the test fails right now for reasons which seem very odd. Just have to keep poking at it.
github-actions
bot
added
the
A3-in_progress
coriolinus
added
B0-silent
drahnr
reviewed
Jul 15, 2020
| let _ = async move { unordered.collect::<Vec<_>>() }.await; | ||
| .collect::<FuturesUnordered<_>>() | ||
| .map(Ok) | ||
| .forward(drain()) |
The root problem here was two-fold: - there was a circular dependency from subsystem -> test-helpers/subsystem -> subsystem - cfg(test) doesn't propagate between crates The solution: move the subsystem test helpers into a sub-module within subsystem. Publicly export them from the previous location so no other code breaks. Doing this has an additional benefit: it ensures that no production code can ever accidentally use the subsystem helpers, as they are compile- gated on cfg(test).
ordian
reviewed
Jul 16, 2020
github-actions
bot
added
A0-please_review
Contributor
Author
|
Log tests weren't the best idea in the first place, so I replaced that whole system in f0c23de |
It's not obvious whether we'll ever really want to chase down these errors outside a testing context, but having the capability won't hurt.
also fix polkadot-node-core-backing tests
drahnr
reviewed
Jul 17, 2020
| rx_to: mpsc::Receiver<Self::ToJob>, | ||
| tx_from: mpsc::Sender<Self::FromJob>, | ||
| receiver: mpsc::Receiver<Self::ToJob>, | ||
| sender: mpsc::Sender<Self::FromJob>, |
drahnr
reviewed
Jul 17, 2020
node/subsystem/src/util.rs
Outdated
| if let Some(mut err_tx) = err_tx { | ||
| // if we can't send the notification of error on the error channel, then | ||
| // there's no point trying to propagate this error onto the channel too | ||
| let _ = err_tx.send((Some(parent_hash), JobsError::Job(e))).await; |
Contributor
There was a problem hiding this comment.
should we print a warning here at least? This seems to be a more general question how we want to handle these cases.
drahnr
reviewed
Jul 17, 2020
Contributor
drahnr
left a comment
drahnr
left a comment
There was a problem hiding this comment.
Mostly one question how we want to handle cases where channels are saturated / unabled to send to the overseer? I am in favor of warn!
rphmeier
reviewed
Jul 17, 2020
rphmeier
reviewed
Jul 17, 2020
Combine tests of starting and stopping job: leaving a test executor with a job running was pretty clearly the cause of the sometimes-hang. Also, add a timeout so tests _can't_ hang anymore; they just fail after a while.
ordian
reviewed
Jul 20, 2020
ordian
reviewed
Jul 20, 2020
| /// | ||
| /// This variant is only valid while testing, but makes the process of testing the | ||
| /// subsystem job manager much simpler. | ||
| #[cfg(test)] |
There was a problem hiding this comment.
do we want #[cfg(any(test, feature = "test-helpers"))] here?
Contributor
Author
There was a problem hiding this comment.
No: this is only used within the internal unit tests of the job manager. If we come up with a use for it later, we can add it then. Until then, I think YAGNI is the better part of valor.
rphmeier
approved these changes
Jul 21, 2020
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
No description provided.