Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

client/finality-grandpa: Add regression test for observer polling network #4778

Closed
wants to merge 1 commit into from

Conversation

mxinden
Copy link
Contributor

@mxinden mxinden commented Jan 30, 2020

Ensure Future implementation of ObserverWork is polling its
NetworkBridge. Regression test for bug introduced in d4fbb89 and
fixed in 7d58cee.

When polled, NetworkBridge forwards reputation change requests from
the GossipValidator to the underlying dyn Network. This test
triggers a reputation change by calling GossipValidator::validate with
an invalid gossip message. After polling the ObserverWork which should
poll the NetworkBridge, the reputation change should be forwarded to
the test network.

Thanks @NikVolf for pushing for this!

Ensure `Future` implementation of `ObserverWork` is polling its
`NetworkBridge`. Regression test for bug introduced in d4fbb89 and
fixed in 7d58cee.

When polled, `NetworkBridge` forwards reputation change requests from
the `GossipValidator` to the underlying `dyn Network`. This test
triggers a reputation change by calling `GossipValidator::validate` with
an invalid gossip message. After polling the `ObserverWork` which should
poll the `NetworkBridge`, the reputation change should be forwarded to
the test network.
// validator to the test network.
assert!(observer.now_or_never().is_none());

match tester.events.next().now_or_never() {
Copy link
Contributor

Choose a reason for hiding this comment

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

assert_matches! would be a better choice here

client/finality-grandpa/src/observer.rs Show resolved Hide resolved

use futures::executor::{self, ThreadPool};

#[test]
Copy link
Member

Choose a reason for hiding this comment

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

Please put bellow the comment.

);

// Trigger a reputation change through the gossip validator.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

let mut tester = executor::block_on(tester_fut);

// Create an observer.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

let thread_pool = ThreadPool::new().unwrap();

// Create a test network.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor

@NikVolf NikVolf left a comment

Choose a reason for hiding this comment

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

lgtm, just couple of nits

@gavofyork
Copy link
Member

@mxinden just needs the nits fixing before it can be merged...

@gavofyork
Copy link
Member

#4795 replaces this.

@gavofyork gavofyork closed this Jan 31, 2020
gavofyork added a commit that referenced this pull request Jan 31, 2020
…work (was #4778) (#4795)

* client/finality-grandpa: Add regression test observer polling network

Ensure `Future` implementation of `ObserverWork` is polling its
`NetworkBridge`. Regression test for bug introduced in d4fbb89 and
fixed in 7d58cee.

When polled, `NetworkBridge` forwards reputation change requests from
the `GossipValidator` to the underlying `dyn Network`. This test
triggers a reputation change by calling `GossipValidator::validate` with
an invalid gossip message. After polling the `ObserverWork` which should
poll the `NetworkBridge`, the reputation change should be forwarded to
the test network.

* Nits

Co-authored-by: Max Inden <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants