embassy-rs / embassy

Modern embedded framework, using Rust and async.
https://embassy.dev
Apache License 2.0
4.91k stars 671 forks source link

Test-friendly embassy-net implementation? #1695

Open jasta opened 12 months ago

jasta commented 12 months ago

I've been looking at integrating nicely with embassy in some async networking libraries I'm working on but ran into a curious issue where I can't run any CI tests against non-trivial embassy code (such as my UdpSocket abstraction). I initially assumed that I just needed to do a bit of legwork in my tests to set up a mock embassy_net_driver but the amount of effort required quickly grew out of control. Am I missing something? How is any of this code tested in CI?

I might be interested in tackling such a project to add a new test-friendly mock implementation to fit alongside existing embassy-net implementations but I'm hoping to get a few pointers from the experts first!

Dirbaio commented 12 months ago

here is an implementation that works on top of linux tun/tap so you can run it on a plain old Linux box. Requires root access though. See this for how to setup routing/bridging.

Alternatively you could write your own and then test sending/receiving packets with some packet manipulation lib. Or make two embassy-net instances talk to each other.

How is any of this code tested in CI?

The bulk of the network stack complexity is in smoltcp, which has a ton of unit tests. embassy-net is just an async wrapper for smoltcp, so testing there has a lower ROI. There are HIL tests that the CI runs on actual hardware, see here and here, these are more intended to test the drivers and not embassy-net itself though.

I might be interested in tackling such a project to add a new test-friendly mock implementation to fit alongside existing embassy-net implementations

The problem with these is the driver sees raw Ethernet packets, so asserting things on those is hard, as I guess you've already seen :sweat: If you want to do stuff like "assert this code connects to $IP and sends $DATA" you might be better off mocking at a layer above embassy-net, such as embedded-io, embedded-nal-async.

jasta commented 11 months ago

FWIW, I got a proof of concept working but it is super hacky and wouldn't scale in a production environment (i.e. memory leaks abound to avoid side effects across test runs):

https://github.com/jasta/coap-server-rs/blob/main/coap-server-embassy/src/transport/udp.rs#L263

To test:

git clone https://github.com/jasta/coap-server-rs
cd coap-server-rs
cargo +nightly test --workspace -p coap-server-embassy

This will run the smoke test linked on your host machine with no side effects other than leaking memory (AFAICT).

A few notes though about what I think would need to be improved in embassy to give this first class support:

  1. New test-friendly embassy-net-driver which removes usage of static lifetimes and offers a graceful shutdown option to exit out of the net_driver loop.
  2. Support for graceful shutdown in embassy_net::Stack::run so that we can exit out of the net_task loop.
  3. Modify or add a companion to Executor::run that exits when there are no more tasks or timers, or maybe just add an explicit way to exit out as per (1) and (2).
  4. Find a way around the requirements of 'static in Stack::new, SocketStack, and *Socket.
  5. Add a [embassy_executor::test] macro to set up all the scaffolding without statics.

As an alternative to all of that, we could add our own custom test harness that forces each test to run in its own subprocess and then just kill it to reclaim memory. Slow and won't scale, but correct and reliable. See for example: https://www.infinyon.com/blog/2021/04/rust-custom-test-harness/

Dirbaio commented 11 months ago

All the 'static requirements are because you're sharing the resources between tasks, and task arguments must be 'static. If you run them in a single task instead (with join/select) there's no such requirement. They can all be in local variables.

You can make Stack::run() exit by canceling the future, for example with select. You don't need it to voluntarily exit.

About the executor: its design fundamentally needs the executor and the tasks to live for 'static (it's what allows it to be so light compared to other executors: others need to refcount tasks/wakers so they don't dangle, while Embassy's can assume they live forever). It's simply not possible to tear down an Embassy executor soundly.

Nothing in embassy-net requires it to run on the Embassy executor though. You can use another executor that's more test-friendly, For example Embassy uses futures_test in some embassy-sync tests.