Tx5Transport Module #75
Conversation
| @@ -99,6 +99,10 @@ fn make_mock_transport( | |||
| } | |||
|
|
|||
| #[tokio::test(flavor = "multi_thread")] | |||
| #[cfg_attr( | |||
There was a problem hiding this comment.
Yeah this is the one thing that's bugging me about K2 at the moment is these flaky tests on Windows. From what I can tell, they are just flaky tests and the code they're testing is fine but I think a good pass over these to make them be more careful with waits would be good @jost-s
They do very occasionally fail elsewhere too. I saw one failure today in maybe 40 test runs. Not bad, but would be good to minimise that.
There was a problem hiding this comment.
I can set all the timeouts to 100 ms. There are no waits that an assertion depends on.
| impl Default for Tx5TransportConfig { | ||
| fn default() -> Self { | ||
| Self { | ||
| server_url: "<wss://your.sbd.url>".into(), |
There was a problem hiding this comment.
Any other way to express this as a required configuration without providing a dummy default?
There was a problem hiding this comment.
My hope was to be able to generate a default conductor config with these defaults in there. I feel like we're going to want an example like this in there when we do that. But I'm open to other suggestions.
There was a problem hiding this comment.
That's fair, we can enforce this higher up. I'm wondering whether Kitsune2 could reject a configuration that doesn't provide a value for this
There was a problem hiding this comment.
For sure there are a couple ways we could do this.
- since the transport library is created in the "create" function of the top-level kitsune module, if we emit errors in them, holochain will get them when it tries to run the builder function, so we can error right on startup
- alternately we could add some kind of "validate_config" option to the factory api, so modules could opt-in to that
There was a problem hiding this comment.
Hmm, the config callback we have is just dealing with setting defaults right? So it would have to be the creates on the factories that would need to check this. That would work and yes Holochain gets the feedback.
I do quite like the idea of a validate option so that we have a clear place to look for config checking rather than hunting through the create code when instances are created. Oh and that also means we should have some easy to call code when we get to updating config. Yeah I think I like that option more
There was a problem hiding this comment.
Probably not a job for this PR then
crates/transport_tx5/src/test.rs
Outdated
| } | ||
| } | ||
|
|
||
| const S1: SpaceId = SpaceId(id::Id(bytes::Bytes::from_static(b"space1"))); |
There was a problem hiding this comment.
There is now a test space in the test_utils module that could be used here
Co-authored-by: ThetaSinner <[email protected]>
| homepage = "https://github.com/holochain/kitsune2" | ||
| documentation = "https://docs.rs/kitsune2_transport_tx5" | ||
| authors = ["Holochain Core Dev Team <[email protected]>"] | ||
| keywords = ["holochain", "holo", "p2p", "dht", "networking"] |
There was a problem hiding this comment.
Do we still need the "holo" keyword in here?
| @@ -99,6 +99,10 @@ fn make_mock_transport( | |||
| } | |||
|
|
|||
| #[tokio::test(flavor = "multi_thread")] | |||
| #[cfg_attr( | |||
There was a problem hiding this comment.
I can set all the timeouts to 100 ms. There are no waits that an assertion depends on.
I've ignored the
peer_connect_disconnecttest due to the following issue: holochain/tx5#119. I think that issue may be a low-ish priority, since we don't yet have any logic that depends on the disconnect events/callbacks.