Add test suite and minor refinements to the utility subsystem #1403

coriolinus · 2020-07-14T14:29:05Z

No description provided.

Unfortunately, the test fails right now for reasons which seem very odd. Just have to keep poking at it.

drahnr · 2020-07-15T11:42:40Z

node/subsystem/src/util.rs

-				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).

node/test-helpers/subsystem/src/lib.rs

coriolinus · 2020-07-17T07:40:38Z

Interestingly, the same test fails in two runs on CI, but I can't get it to fail locally on Ubuntu or OSX over several runs. I'd prefer not to just delete the test. Does anyone here have experience with failures like this?

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.

…-test-suite

also fix polkadot-node-core-backing tests

drahnr · 2020-07-17T15:26:35Z

node/subsystem/src/util.rs

-		rx_to: mpsc::Receiver<Self::ToJob>,
-		tx_from: mpsc::Sender<Self::FromJob>,
+		receiver: mpsc::Receiver<Self::ToJob>,
+		sender: mpsc::Sender<Self::FromJob>,


drahnr · 2020-07-17T15:27:18Z

node/subsystem/src/util.rs

+				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;


should we print a warning here at least? This seems to be a more general question how we want to handle these cases.

drahnr

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!

node/subsystem/src/util.rs

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.

…-test-suite

node/subsystem/src/lib.rs

ordian · 2020-07-20T11:25:02Z

node/subsystem/src/messages.rs

+	///
+	/// This variant is only valid while testing, but makes the process of testing the
+	/// subsystem job manager much simpler.
+	#[cfg(test)]


do we want #[cfg(any(test, feature = "test-helpers"))] here?

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.

coriolinus added 2 commits July 14, 2020 16:28

get conclude signal working properly; don't allocate a vector

c72e59c

wip: add test suite / example / explanation for using utility subsystem

00bbe47

Unfortunately, the test fails right now for reasons which seem very odd. Just have to keep poking at it.

coriolinus self-assigned this Jul 14, 2020

github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jul 14, 2020

coriolinus added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 15, 2020

drahnr reviewed Jul 15, 2020

View reviewed changes

coriolinus added 2 commits July 16, 2020 12:47

explicitly import everything

837a62a

ordian reviewed Jul 16, 2020

View reviewed changes

node/test-helpers/subsystem/src/lib.rs Outdated Show resolved Hide resolved

coriolinus added 2 commits July 16, 2020 16:21

fully commit to moving test helpers into a subsystem module

3ff4f66

add some more tests

356c26f

coriolinus marked this pull request as ready for review July 16, 2020 15:08

github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 16, 2020

coriolinus added 5 commits July 17, 2020 13:54

get rid of log tests in favor of real error forwarding

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.

Merge remote-tracking branch 'origin/master' into prgn-subsystem-util…

68a745d

…-test-suite

fix issue which caused test to hang on osx

66e7d50

only require that job errors are PartialEq when testing

a9b49af

also fix polkadot-node-core-backing tests

get rid of any notion of partialeq

812b35a

coriolinus requested review from rphmeier and drahnr July 17, 2020 14:57

drahnr reviewed Jul 17, 2020

View reviewed changes

rphmeier reviewed Jul 17, 2020

View reviewed changes

node/subsystem/src/util.rs Outdated Show resolved Hide resolved

rphmeier reviewed Jul 17, 2020

View reviewed changes

node/subsystem/src/util.rs Outdated Show resolved Hide resolved

coriolinus added 2 commits July 20, 2020 08:54

rethink testing

7e758b5

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.

rename fwd_errors -> forward_errors

6672e2a

coriolinus added 3 commits July 20, 2020 09:01

warn on error propagation failure

6b056bf

Merge remote-tracking branch 'origin/master' into prgn-subsystem-util…

f3aa719

…-test-suite

fix unused import leftover from merge

bcd0a6d

coriolinus mentioned this pull request Jul 20, 2020

implement bitfield signing subsystem #1364

Merged

ordian reviewed Jul 20, 2020

View reviewed changes

node/subsystem/src/lib.rs Outdated Show resolved Hide resolved

derive eq for subsystemerror

5b464b0

ordian reviewed Jul 20, 2020

View reviewed changes

rphmeier approved these changes Jul 21, 2020

View reviewed changes

rphmeier merged commit cc2d7af into master Jul 21, 2020

rphmeier deleted the prgn-subsystem-util-test-suite branch July 21, 2020 00:35

ordian mentioned this pull request Jul 31, 2020

Break subsystem-util out into a separate crate and restore the subsystem test-helpers crate #1515

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test suite and minor refinements to the utility subsystem #1403

Add test suite and minor refinements to the utility subsystem #1403

coriolinus commented Jul 14, 2020

drahnr Jul 15, 2020

coriolinus commented Jul 17, 2020 •

edited

Loading

drahnr Jul 17, 2020

drahnr Jul 17, 2020 •

edited

Loading

drahnr left a comment

ordian Jul 20, 2020

coriolinus Jul 20, 2020

Add test suite and minor refinements to the utility subsystem #1403

Add test suite and minor refinements to the utility subsystem #1403

Conversation

coriolinus commented Jul 14, 2020

drahnr Jul 15, 2020

Choose a reason for hiding this comment

coriolinus commented Jul 17, 2020 • edited Loading

drahnr Jul 17, 2020

Choose a reason for hiding this comment

drahnr Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

drahnr left a comment

Choose a reason for hiding this comment

ordian Jul 20, 2020

Choose a reason for hiding this comment

coriolinus Jul 20, 2020

Choose a reason for hiding this comment

coriolinus commented Jul 17, 2020 •

edited

Loading

drahnr Jul 17, 2020 •

edited

Loading