test(clitools): revive run_inprocess()

[rami3l]

This PR re-enables testing via the run_inprocess() function, which was originally introduced in #2367 but temporarily disabled in #3868.

Please note that this is a dirty fix as so far all in-process tests are still synchronous, and a tokio runtime builder has been re-introduced to handle the async-ness of rustup_init::main() as well as the need of initializing an otel subscriber in an async context.

[djc]

Please note that this is a dirty fix as so far all in-process tests are still synchronous,

What do you mean? It seems pretty mucht right to me.

[rami3l]

Please note that this is a dirty fix as so far all in-process tests are still synchronous,

What do you mean? It seems pretty mucht right to me.

@djc The previous implementation used a unique runtime and a static TRACER that is initialized in the former for all in-process tests. However once process is involved that seems no longer possible.

[djc]

What do you mean? It seems pretty mucht right to me.

@djc The previous implementation used a unique runtime and a static TRACER that is initialized in the former for all in-process tests. However once process is involved that seems no longer possible.

Do you mean because I've tied the DefaultGuard into the TestContext? That's something we can just change if we want to. I suppose the issue here is that we have different kinds of tests that need the subscriber, these in-process tests but also "simpler"/more unit-style tests that benefit from the current setup. However we could for example add a third variant to Process for this maybe?

[rami3l]

Do you mean because I've tied the DefaultGuard into the TestContext? That's something we can just change if we want to. I suppose the issue here is that we have different kinds of tests that need the subscriber, these in-process tests but also "simpler"/more unit-style tests that benefit from the current setup. However we could for example add a third variant to Process for this maybe?

@djc Indeed, that would be interesting.

One trap I've encountered here is that currently TestContext cannot be constructed without an async context (or it'll panic), and that's why a big async block is required in the code above. Not surprising to me, but it's still a bit easy for me to accidentally make the mistake of initializing it at a random place and produce a panic (x_x)

[djc]

Do you mean because I've tied the DefaultGuard into the TestContext? That's something we can just change if we want to. I suppose the issue here is that we have different kinds of tests that need the subscriber, these in-process tests but also "simpler"/more unit-style tests that benefit from the current setup. However we could for example add a third variant to Process for this maybe?

@djc Indeed, that would be interesting.

One trap I've encountered here is that currently TestContext cannot be constructed without an async context (or it'll panic), and that's why a big async block is required in the code above. Not surprising to me, but it's still a bit easy for me to accidentally make the mistake of initializing it at a random place and produce a panic (x_x)

We could explicitly stick a Runtime in it, but it feels like overkill? Ideally in my mind we would not have the Runtime construction in the code and instead annotate all tests that need it with #[tokio::test], which seems like the idiomatic approach to me.

[rami3l]

Ideally in my mind we would not have the Runtime construction in the code and instead annotate all tests that need it with #[tokio::test], which seems like the idiomatic approach to me.

@djc That's also where I want it to be, however the current state is still far from it.

[djc]

Ideally in my mind we would not have the Runtime construction in the code and instead annotate all tests that need it with #[tokio::test], which seems like the idiomatic approach to me.

@djc That's also where I want it to be, however the current state is still far from it.

Why far? If we just remove the explicit runtime builder and add #[tokio::test] to the tests that need it, what breaks?

[rami3l]

Ideally in my mind we would not have the Runtime construction in the code and instead annotate all tests that need it with #[tokio::test], which seems like the idiomatic approach to me.

@djc That's also where I want it to be, however the current state is still far from it.

Why far? If we just remove the explicit runtime builder and add #[tokio::test] to the tests that need it, what breaks?

@djc Nothing in theory. However, as I've explained already, it's just that run_inprocess() is a bit deep down already in the call chain, and it's only there we start to observe asyncness (meaning only its callees can be async), so as for the current solution all its callers like test() and .run() don't require any changes.

OTOH, actually making the tests async (and adding #[tokio::test] to them) requires a nontrivial amount of effort to migrate those helper functions to async as well (which is worked around in this PR by pushing back the starting point of asyncness). But maybe you can have a try after this PR? I could be doing it wrong from the beginning, I'm not quite sure...

Anyway, I think this PR is ready to go as a first step. Please feel free to propose improvements in the subsequent ones.