[ECS] Infra for comprehensive unit tests

[dyc3]

Motivation: There are a lot of edge cases we have to handle, and every new line of code we write increases the risk of regressions. Regressions slow down dev time: a perfect example of this is what happened in #189.

Unsolved issues:

• test_client_position is flakey

Test plan

cargo test -p valence_new

[rj00a]

If we are to move forward with my proposal in #199, here is what I think should happen (in no particular order):

• Move all of Valence's tokio code and initial login procedures into a separate crate, let's call it valence_network. valence should no longer have a dependency on tokio. It is the responsibility of valence_network to spawn Client entities as connections are made to the server. valence_network exposes a Bevy Plugin.
• The OwnedSemaphorePermit that is stored in Client can be moved into a separate component defined in valence_network.
• Client's constructor now looks like this:

  pub fn new(info: NewClientInfo, stream: Box<dyn PacketStream>) -> Self { ... }

• PacketStream is a new trait that has try_send and try_recv methods similar to byte channels.

With those things done, setting up test infra should be easy. How much of that you want to address here or elsewhere is up to you.

[rj00a]

Actually I just realized it would make more sense to put the semaphore permit in valence_network's impl of PacketStream so it can't be lost.

[dyc3]

I've moved mostly everything related to accepting connections into valence_network. There's a few unsolved problems that I might need help with:

• what to do with dimensions and biomes?
• we need some way to configure a bundle to be added with a client, or defer actually creating the entities to a system in valence.
  • Because client entities need to also have an Inventory component, and importing valence would be a circular dep tree

[rj00a]

Let me lay down a few things just so we're on the same page.

• valence_network depends on valence and not the other way around.
• valence_network handles the "handshake", "status", and "login" stages of the Minecraft protocol. valence handles "play".
• Dimensions and biomes stay in valence.
• All of the stuff in byte_channel.rs, connect.rs, and packet_manager.rs goes in valence_network.
• Your config.rs in valence_network looks correct.
• PlayPacketSender/PlayPacketReceiver are mostly just useless wrappers at this point and can go away.
• Within valence we can probably get rid of SharedServer and just merge all the remaining fields into Server.
• Here are the fields of Server that should end up in valence_network:
  • address
  • connection_mode
  • compression_threshold (probably needs to be copied in both places)
  • max_connections
  • incoming_capacity
  • outgoing_capacity
  • tokio_handle
  • tokio_runtime
  • new_clients_send
  • new_clients_recv
  • connection_sema
  • rsa_key
  • public_key_der
  • http_client
• Client should have a PacketEncoder and PacketDecoder fields named enc and dec.
• Flushing the client's packet buffer involves calling try_send on the client's stream.
• PacketEncoder should implement the WritePacket trait.
• To create a new client, we need to know what the configured compression threshold is. Therefore we probably need a new_client method on Server, similar to new_instance. (Adding a compression_threshold argument to Client::new wouldn't be great because there is a possibility to pass in the wrong argument.)
• Here are the dependencies that can be moved into valence_network:
  • async-trait
  • flume
  • hmac
  • num-bigint (for the auth digest)
  • rand (needed in both)
  • rsa
  • rsa-der
  • serde_json
  • sha1
  • sha2
  • tokio

Hopefully there aren't any major issues I'm overlooking.

[rj00a]

Just a thought: The ability to enable/disable plugins in a general way in the "stageless" rearchitecture of Bevy (coming in the next release) would make a lot of sense for toggleable "Open to LAN" functionality in singleplayer mode. And even in regular servers I could see it being useful too.

[dyc3]

From discussion on discord: this valence_network refactor is too complex for me to handle right now, so I'm just going to be adding the PacketStream trait and refactoring to integrate that.

[rj00a]

Some things I'd love to be able to test once this is complete because the entity/chunk code is horribly complicated:

• Entity/chunk update packets are only sent for entities/chunks that are considered loaded by the client.
• Entities and chunks are not unloaded twice.
• Entities and chunks are unloaded when leaving the client's view.

[rj00a]

I've implemented my previous suggestions along with the ClientConnection trait in 0b4e908 because it was quite a bit more involved than I was imagining.

[rj00a]

Sorry about the merge conflicts 😬

[dyc3]

There's a couple of weird quirks that I've discovered when working on example_test_open_inventory. And they somewhat harm dev ergonomics for unit tests.

• If the client is not assigned an instance as soon as it connects, update_one_client fails the first check, and the client is immediately disconnected.
• MockClientHelper is unable to expose collect_sent without doing some lifetime voodoo. (Likely a similar problem as before when collect_sent wouldn't compile.)

And regarding the merge conflicts, it seems like I'm probably going to have to rewrite most of this PR to make it work again.

[rj00a]

If the client is not assigned an instance as soon as it connects, update_one_client fails the first check, and the client is immediately disconnected.

Perhaps you could wrap up all the initialization boilerplate (create an app, create an instance and client, add client to instance, etc.) in a helper function? Immediately disconnecting the client when not in an instance was a deliberate design choice, but maybe we could afford to be a bit lax there.

[dyc3]

I think this is ready to review. I'm happy with the verbosity of the example tests. It should be sufficient to test that chunk stuff.

[rj00a]

Looking good.