tokio-rs / turmoil

Add hardship to your tests
MIT License
795 stars 50 forks source link

Enable tokio io driver #171

Closed HolyShitMan closed 7 months ago

HolyShitMan commented 8 months ago

Pull request includes:

mcches commented 8 months ago

This has come up a couple of times in the past. We intentionally don't enable io to avoid confusion between tokio::net and turmoil::net types. Can I ask why you aren't using turmoil::net types in your tests and for a little more information about your use case?

HolyShitMan commented 8 months ago

We certainly understand this intention. We have currently two use-cases, that use the tokio io types:

  1. Manual GUI testing framework that uses turmoil internally and exposes one socket externally to attach GUI.
  2. Tests that write files onto the HDD (missed this in my initial comment onto the PR)

Maybe a compromise could be to hide the IO driver activation behind a feature flag?!

mcches commented 8 months ago
  1. Manual GUI testing framework that uses turmoil internally and exposes one socket externally to attach GUI.

Interesting! I'd love to hear more about this or see a demo.

  1. Tests that write files onto the HDD (missed this in my initial comment onto the PR)

This should be fine if you use tokio fs as that doesn't use the io reactor.

Maybe a compromise could be to hide the IO driver activation behind a feature flag?!

I'd be more comfortable doing it like this.

mcches commented 8 months ago

You could also smuggle messages out via a channel and then run your socket independently of turmoil.

HolyShitMan commented 8 months ago

You could also smuggle messages out via a channel and then run your socket independently of turmoil.

This of course could also be possible. Any strong opinion from your side, whether to include this PR?

HolyShitMan commented 8 months ago

We have currently two use-cases, that use the tokio io types:

I ask the college again on his problems and I misunderstood him. The problem weren't the files, but he ran a command which uses pipes internally. And this led to a tokio crash without IO feature activated.

mcches commented 8 months ago

You could also smuggle messages out via a channel and then run your socket independently of turmoil.

This of course could also be possible. Any strong opinion from your side, whether to include this PR?

If it unblocks you and it isn't too much of a hassle I'd prefer that approach. One reason for this is that at some point down the line we'd like to incorporate this into tokio itself, at which point you'd set a flag on the runtime and you get one or the other. I'm not sure when that will be, but if we can avoid it now that would save you some upgrade headache later.

HolyShitMan commented 8 months ago

I talked to all my colleges, that are involved in this issue of needing IO within unit tests, and the consensus was, that by not using the tokio IO driver the short and medium term effort is to high and our code base readability decreases massively. Still, we fully understand your point, and if you're not inclined to merge this PR, we propose two alternatives:

  1. I withdraw this PR and create a new one, introducing an additional constructor that accepts a user-created tokio runtime as a parameter. Maybe this aligns better with your long-term goals.
  2. If this isn't feasible, we'll continue using our local turmoil fork
mcches commented 8 months ago

Let's do a Builder option and proceed with enabling it. This has the advantage of being very explicit and allows for tests to have it on and off in the same crate.

HolyShitMan commented 8 months ago

Not completely sure about everything, e.g., building no_software runtime with Config::default();, yet count this as a first version to discuss about.

mcches commented 7 months ago

Merged. I will release this by eow.

HolyShitMan commented 7 months ago

thx for your work