NDRS-119: add deploy gossiper tests

[Fraser999]

This adds some integration tests for the deploy gossiper component.

Further tests should ideally be added in future to check handling of e.g. invalid/malicious gossip messages. Such tests would likely need the existing test network setup to be extended somewhat, and is outside the scope of this PR.

Note that the DeployGossiper::Event and DeployGossiper::Message were simply moved into new files without any material changes.

The test framework underwent some changes:

• fixed bug where lazy_static! was used instead of thread_local!
• fixed bug whereby a test node would send network messages to itself
• extended to support injection of specific events into a specific node's event queue (this allows the test to trigger a gossip flow by injecting a DeployReceived event)
• extended to support removal of nodes (this allows the test to check for timeout handling of non-responsive nodes)
• improved the settle and settle_on methods by adding a timeout parameter to avoid tests hanging indefinitely

[goral09]

@Fraser999 Could you try creating PRs with more granular changes? For example, the list you made each point could be a separate commit etc.

[goral09]

We're using both criterion and proptest. Is there a reason why we can't use just one?

[Fraser999]

They do different things really. Criterion is a benchmarking tool, whereas proptest is for testing properties - exercising your API by trying various randomised inputs.

[goral09]

Ah, thanks. I misremembered something then.

[marc-casperlabs]

I think this has a half a point. We'll want a proptest-strategy to create random Deploys. There's merit in having this function to have it called by said strategy though, as it would allow us to create random deploys outside that particular use case.

[goral09]

Truth is that this deploy is not random at all.

[Fraser999]

@goral09

Could you try creating PRs with more granular changes? For example, the list you made each point could be a separate commit etc.

I'll try. I guess I could have broken this down into a few commits as I progressed through writing the test cases. However, something like the lazy_static bug only became apparent once I'd finished all the tests and ran them in parallel. If I'd made the fix a final separate commit, it would mean the previous commits would produce failing tests.

But I take your point - I'll try and split PRs into smaller ones than this.

[marc-casperlabs]

Biggest blockers for me are the reinvention of proptest and event injection.

[marc-casperlabs]

I understand why this is desired, but I believe I removed it because it made it not show up in the docs, IIRC. Also, I do not think doctests are executed in this case.

Correct me if I am from, but this should be conditional on both test and doctest? (#[cfg(any(test, doctest))])

[Fraser999]

I think the consensus so far appears to be that we shouldn't add modules/types to public APIs unless required. After the proposed style guide is finalised, if we do prefer to extend APIs just to support docs/doctests we can change this, but for now I'd rather keep the test-only code test feature-gated.

If we take that view, I don't think there's any point to changing this to #[cfg(any(test, doctest))] since doctests won't run this code?

[marc-casperlabs]

doctest will cause it to run with cargo test AFAIK.

[Fraser999]

It tried and it seems like it doesn't.

[marc-casperlabs]

This whole test function reinvents proptest, which is already a dependency!

I would rather see a single test function that takes network size and deploy count as a parameter and runs one test. Even if we're not using proptest, this should be called instead.

That function should also create a new network controller each time, obliviating the need for the Drop implementation above.

[marc-casperlabs]

To reiterate, I think this whole test should be refactored as follows:

• Create a strategy for creating a random DeploySomething (not the actual name, just the thing we actually require to test gossiping)
• Create run_gossip_test(network_size: usize, random_deploys: Vec<DeploySomething>) function that runs the test:
  1. Create new network
  2. Create network_size nodes
  3. Distribute random_deploys to network nodes
  4. Run network and check for completion
• Create the test function that calls run_gossip_test and receives values for network_size, etc. from proptest. (e.g. proptest! { fn should_gossip(network_size in 0..20) ... }. Note that this proptest call could be on run_gossip_test directly, but I typically add this indirection, because rustfmt won't format code inside a proptest! macro call =).

[Fraser999]

This whole test function reinvents proptest

This is a long, long way from re-inventing proptest! :D

The test has been structured to allow easy transition to proptest should we decide to refactor in that direction in the future.

I did consider using proptest for this, but decided against it for now since I think it would be overkill for this scenario. The only parameters worth varying are the network size and deploy count. These have such low ranges that it's not worth the cost in terms of time to run a proptest to completion.

I think we should reserve usage of proptest to places where we really feel it's worthwhile, both in terms of coding effort, but more in terms of test execution time. If others agree with you that this is one such place, then I'll certainly make the change.

[marc-casperlabs]

Why would a proptest here take more time? You can limit the number of cases generated (the setting is called cases IIRC). Right now, the implementation here uses 1 instance out of 0..20 and it won't even save the inputs that caused the issue when rerunning. Set proptest's cases to 1 and you at least get that one for free.

Other than that, you'll only pay a price when your test fails and is run again in an attempt to mimize, but I'd argue that's time well spent =)

Also, just how long does this test take? Surely only a few milliseconds?

[Fraser999]

I expect we'll have a test framework (soon) which allows us to display a seed on test failure and reproduce the test failure reliably without needing that functionality to be provided by proptest.

Setting up proptest for this test case to only run it once still seems like overkill to me.

[marc-casperlabs]

If setting up the network is a common issue, we should either create a convenience function here or consider adding a function to create multiple nodes to Network.

[Fraser999]

I'll add a function to Network since I expect it will be used by other integration tests.

[marc-casperlabs]

Adding a crank-single-node function is a really good idea, in case we want to test for a specific race condition 👍

[marc-casperlabs]

Let's .expect here regardless?

[Fraser999]

I don't think we win anything using expect() in a test expression which only has a single point of panic. The outputted line number serves as well or better than the expect string I think.

I'll make the change anyway :)

[marc-casperlabs]

If we have a single-node crank function, maybe crank_all shoud be rewritten to use it?

[Fraser999]

That was my initial thought too, but the borrow checker wasn't a fan of iterating via self.nodes.values_mut() and trying to call crank(&mut self, ...).

[marc-casperlabs]

See above, can we not do without the injection?

[marc-casperlabs]

I think this has a half a point. We'll want a proptest-strategy to create random Deploys. There's merit in having this function to have it called by said strategy though, as it would allow us to create random deploys outside that particular use case.

[marc-casperlabs]

The injection issue has been resolved, and the other stuff has been put on the docket for later. We can merge this under these circumstances. =)