Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Restore stability of peers blacklists and disconnections in Network Tests - Closes #2670 #2725

Merged
merged 10 commits into from
Jan 10, 2019

Conversation

diego-G
Copy link

@diego-G diego-G commented Jan 9, 2019

How to test it?

npm test -- mocha:default:network

Review checklist

@diego-G diego-G self-assigned this Jan 9, 2019
@MaciejBaj MaciejBaj assigned 2snEM6 and unassigned 2snEM6 Jan 9, 2019
@MaciejBaj MaciejBaj requested a review from 2snEM6 January 9, 2019 14:32
@MaciejBaj MaciejBaj changed the title Restore stability of peers blacklists and disconnections in Network Tests - Close #2670 Restore stability of peers blacklists and disconnections in Network Tests - Closes #2670 Jan 9, 2019
Copy link
Contributor

@MaciejBaj MaciejBaj left a comment

Choose a reason for hiding this comment

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

What we've agreed to change is:

  • To test disconnect/connect, rely on open connections numbers, but not strictly - should.be.above is enough as the connections with other peers are just propagating
  • Due to the indeterministic propagation of connections with other peers after executing connection/disconnection network tests, checking the number of open connections with the other peers is pointless while testing peers blacklist. Check only the "BANNED" state while blacklisting.

Both of those conclusions are not introduced.
Peer disconnection tests are still relying on the strict numbers, but now the timeout is extended from 2 to 4 blocks, peers blacklists are still checking the number of connections, not the statuses only.

@diego-G diego-G force-pushed the 2670-change_network_test_approach branch from 397c04a to acd6f58 Compare January 10, 2019 09:00
@diego-G
Copy link
Author

diego-G commented Jan 10, 2019

@MaciejBaj every existing test is meaningful. Remove them or skip them is not the solution. The idea is to reduce the strictness on those problematic tests but, as less as possible while keeping a good balance in between test execution timing and stability. As you can realise the Jenkins trend build, I have found a good stability. It is reasonable and stable to be merged.

I want to state again this test suite is very useful for developers. It just needs more love. We should plan to seriously improve it for the future. Several points to perform are:

  • Execute every test independently from each other : i.e, reinitiated the network completely after every scenario
  • Execute scenarios in parallel to speed up the process : right now they are being executed in sequence over the same network. We might create several networks, for instance 1 per well-defined scenario.
  • Review the design from scratch and find more and better use cases
  • When a test failure occurs, gathering more data from the network status to reproduce the case and debug it

@MaciejBaj MaciejBaj merged commit 9f60c09 into development Jan 10, 2019
@diego-G diego-G deleted the 2670-change_network_test_approach branch January 16, 2019 08:52
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.

Restore stability of peers blacklists and disconnections in Network Tests
3 participants