Conversation
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
90% looks great, thank you!
| /// See documentation for [`PebbleEnvironment`]. | ||
| #[tokio::test] | ||
| #[ignore] | ||
| async fn http_01() -> Result<(), Box<dyn StdError>> { |
There was a problem hiding this comment.
I think this is going to be problematic looking forward to adding more tests. My intent was to have main spin up one Pebble environment and then serially invoke each test.
I'm not a fan of this approach because if tests are run in parallel there will be port conflicts spinning up multiple pebble environments with the same config. Using a different config per test might be workable, but it ratchets up the complexity for little gain IMO.
I was initially thinking of having separate #[test]'s but a single global pebble environment but without using something like Nextest I found the Rust unit test runner too primitive to be able to nicely handle setting up/tearing down a global instance reliably.
Sorry, should have put more of this context in the original desc.
There was a problem hiding this comment.
So IMO the previous setup has a big problem, which is that by default, cargo c/cargo t/cargo clippy don't "see" the code. This makes the IDE experience janky and also resulted in clippy issues slipping through CI. So, I'm open to doing it some other way, but I feel strongly about keeping the code in a place where Cargo can see it by default.
Maybe a solution might be to stick the PebbleEnvironment in a LazyLock and allow tests to work from there?
There was a problem hiding this comment.
So IMO the previous setup has a big problem, which is that by default, cargo c/cargo t/cargo clippy don't "see" the code. This makes the IDE experience pretty janky and also resulted in clippy issues slipping through CI.
Hm yeah, that is crummy. I hadn't noticed that.
What about removing test = false and then making main() skip the test with a message if it doesn't find the required binaries? based on some quick local testing removing that Cargo.toml bit seems to do the trick w.r.t clippy.
Maybe a solution might be to stick the PebbleEnvironment in a LazyLock and allow tests to work from there?
I actually tried this initially but found that static's aren't dropped and so it was leaving behind pebble processes. As far as I could tell there wasn't any way to know when all the tests were done running to do it manually :-/
There was a problem hiding this comment.
What about removing
test = falseand then makingmain()skip the test with a message if it doesn't find the required binaries? based on some quick local testing removing thatCargo.tomlbit seems to do the trick w.r.t clippy.
I did also consider that, but I'm worried that it'll be easy to miss when the tests just don't run, since there is no kind of "skipped" state in the Cargo test framework.
There's a suggestion here that we could use an atomic counter to drop the processes when the counter goes down to zero, maybe that is a feasible solution?
There was a problem hiding this comment.
There's a suggestion here that we could use an atomic counter to drop the processes when the counter goes down to zero, maybe that is a feasible solution?
That's a decent idea. It sounds workable to me. Do you want to run with it or should I?
There was a problem hiding this comment.
I'm running low on time for today. Suggest we merge this and you can pick it up, or I might have a look tomorrow. Got a very sick kid so kinda constrained.
There was a problem hiding this comment.
Sounds good, best of luck to you & kiddo!
2 participants
@cpu happy to get your feedback!