Adding testing infrastructure to support sharded tests #1392
Conversation
| assert_eq!(apps[i * 3 + j].mpc_server.config.port, Some(port)); | ||
| assert_eq_configs( | ||
| &apps[i * 3].mpc_network_config, | ||
| &apps[i * 3 + j].mpc_network_config, |
There was a problem hiding this comment.
I remember you added a function to compute that index, would it work here?
There was a problem hiding this comment.
Since I'm enumerating over the ports it won't be straight forward. I don't want to iterate over the computed results since they could have a problem (perhaps a missing ring), I rather iterate over the "known" result.
| A1UEAwwJbG9jYWxob3N0MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEyzSofZIX | ||
| XgLUKGumrN3SEXOMOAKXcl1VshTBzvyVwxxnD01WVLgS80/TELEltT8SMj1Cgu7I | ||
| tkDx3EVPjq4pOKNHMEUwFAYDVR0RBA0wC4IJbG9jYWxob3N0MA4GA1UdDwEB/wQE | ||
| MIIBZTCCAQugAwIBAgIITIDzw5k9qXIwCgYIKoZIzj0EAwIwFDESMBAGA1UEAwwJ |
There was a problem hiding this comment.
what drove this change?
There was a problem hiding this comment.
I think I had just copied it wrong in the first place and only now that I'm actually exercising it, saw the error
ipa-core/src/net/test.rs
Outdated
| @@ -45,6 +46,38 @@ pub const DEFAULT_TEST_PORTS: Ports = Ports { | |||
| shards: [6000, 6001, 6002], | |||
| }; | |||
|
|
|||
| /// A network with two shards per helper. | |||
| pub const TWO_SHARDS: [Ports; 2] = [ | |||
There was a problem hiding this comment.
are we using this to spin up an actual server in unit tests? if so, I don't think we should do it - we have to pick ports randomly
There was a problem hiding this comment.
We have both tests. I can remove the one using this set.
There was a problem hiding this comment.
I looked at the change and I don't understand whether we ever create a server with hardcoded ports or no. My line of thinking is that we should never need that - hardcoded ports are only good for config validation, everything else should use ports assigned by Kernel
There was a problem hiding this comment.
Yes, I modified the code so that we only use specified ports for config unit tests. All the HTTP tests now use OS given ports.
ipa-core/src/net/test.rs
Outdated
| @@ -87,17 +120,18 @@ impl AddressableTestServer { | |||
| /// Either a single Ring on MPC connection or all of the shards in a Helper. | |||
| pub struct TestNetwork<F: ConnectionFlavor> { | |||
| pub network: NetworkConfig<F>, // Contains Clients config | |||
| pub servers: Vec<AddressableTestServer>, | |||
| pub servers: Vec<Option<AddressableTestServer>>, | |||
There was a problem hiding this comment.
is it what you want, or Option<Vec<..>> represents the state where you don't have ports assigned better?
There was a problem hiding this comment.
Not really, in here is more of a rust ownership constraint.
I need to be able to take the server/ports out of this struct.
There was a problem hiding this comment.
this to me indicates that there is something off here. Consider consuming self and using servers.into_iter() to get AddressableTestServer instead of &mut Option<AddressableTestServer> that you can later replace
There was a problem hiding this comment.
Sounds good. This was a vestigial change before I used destructuring and zip in the to_app function. It was simple to remove. Good catch!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1392 +/- ##
=======================================
Coverage ? 93.65%
=======================================
Files ? 223
Lines ? 37750
Branches ? 0
=======================================
Hits ? 35356
Misses ? 2394
Partials ? 0 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks for addressing comments @cberkhoff, I like the conciseness of this change now. Great job simplifying the code
Now we can start a sharded infrastructure for tests. I'm using the following changes to test the upcoming Query Workflow. I'm front-loading the tests that I will later use to make sure everything keeps working on every PR.
Created the
TestAppstruct which is used to createHelperAppby HTTP tests. HTTP Transport tests uses this new struct to spin up the hosts.One of the certificates I had uploaded before was wrong so I'm updating it now.
I plan to breakdown
tests.rsin a later PR, once the changes I have in the backlog are merged.