Add a black box local cluster harness #3028
Conversation
aeyakovenko
force-pushed
the
local_cluster_tests
branch
from
8c27199 to
2658fef
Compare
Codecov Report
@@ Coverage Diff @@
## master #3028 +/- ##
========================================
+ Coverage 76.8% 77.3% +0.4%
========================================
Files 130 131 +1
Lines 20039 19892 -147
========================================
- Hits 15404 15377 -27
+ Misses 4635 4515 -120 |
|
Can you put a hypothetical Greg hat on for a minute with focus on naming and consistentcy? I'll review after your next commit. |
src/cluster_tests.rs
Outdated
| use solana_sdk::system_transaction::SystemTransaction; | ||
|
|
||
| /// Spend and verify from every node in the network | ||
| pub fn spend_and_verify_all_nodes(entry: &ContactInfo, alice: &Keypair, nodes: usize) { |
There was a problem hiding this comment.
Can you remove the converge() call and move the remaining function into client.rs? Maybe a converge_and_verify function would be helpful too, but that would-be 2-liner probably doesn't need a whole module to itself. Maybe put it in tests/local_cluster.rs?
There was a problem hiding this comment.
@garious the entry point needs to discover the network. maybe rename converge to discover?
src/local_cluster.rs
Outdated
| use std::thread::JoinHandle; | ||
|
|
||
| pub struct LocalCluster { | ||
| pub mint: Keypair, |
There was a problem hiding this comment.
@garious funding_keypair? This keypair is intended for the consumer of the LocalCluster to enter the network
|
Biggest naming nit here is the mix of the term "cluster" and "network" in the same module. Looks like every use of "network" can either be "cluster" or deleted. |
| ledger_paths.push(ledger_path.clone()); | ||
|
|
||
| // Send each validator some tokens to vote | ||
| let validator_balance = Self::transfer( |
There was a problem hiding this comment.
I think it's not creating the vote accounts for the validators. Now the balance of vote account is used to calculate node stakes (for leader rotations etc).
There was a problem hiding this comment.
@pgarg66 can you show me?
There was a problem hiding this comment.
This function (or something similar) needs to be called here for every validator node
https://github.com/solana-labs/solana/blob/master/fullnode/src/main.rs#L51
There was a problem hiding this comment.
This PR changes the way stakes are calculated #2992
Earlier it was using node's account balance. Now it's using staking (voting) account balance.
There was a problem hiding this comment.
@pgarg66 cool! can you add it?
* extract epoch-info crate * Update description Co-authored-by: Joe C <[email protected]> --------- Co-authored-by: Joe C <[email protected]>
Problem
Multinode tests are not true integration tests. Changing an implementation detail without actually breaking functionality breaks tests that verify cluster functionality. The tests are huge, and a painful to maintain.
Summary of Changes
Add a harness to write tests against the public interface to the network.
Our code base should contain 2 kinds of tests
Pure unit tests - These tests cover the implementation details of the function. If the implementation details change, these tests can be deleted and new ones added if necessary. There is no backwards compatibility requirement for unit tests.
Pure integration tests - They start form a cluster entry point, and a keypair with some funds. These tests cover network functionality against the public interfaces available on the network. The tests are written in such a way that they can run against a public testnet or a local cluster. Just like unit tests, they check 1 thing at a time against the network.
Not every feature needs an integration test. Especially things that are not fully baked. But all code needs unit tests.
Fixes #