Any suggested pattern for closing the Client on test failure?

[mpalmer]

(I'm not a fan of "question" issues, but I'm at the end of my tether here, so please forgive me. In my defence, if I can get even the slightest hint towards a solution to my problem, I promise a lovingly hand-crafted docs PR that will make old men weep and small children stare in awe.)

I've started using fantoccini for doing headless UI testing, and it's wonderful. Knowing that I'm testing against real browsers, doing real things, with JS and everything, is absolute awesomesauce. I've found fantoccini easy as heckfire and delightful to use. Thank you for building it.

Except! Whenever a test fails for any reason, that means that the client doesn't get closed, which means the session never gets closed, which means that all the bad things predicted in the Client::close() docs happen.

Since Rust's whole testing philosophy is predicated on "assertion failed? time to bail!", I figure I'm not going to be able to avoid the panic on failure issue, so I need a workaround so that I don't have to constantly restart geckodriver. However, so far, I haven't been able to actually make anything work. On the assumption that I'm not the first person to have come up against this, and that I'm absolutely terrible with anything involving async code, I'm assuming there's a solution out there.

What I've tried so far:

• Using the AsyncDrop trait (which has only just recently appeared) on a wrapper type. I got that to compile, but the async_drop method never seemed to ever actually get called. I don't like relying on unstable features anyway, so I didn't spend ages on trying to figure it out.
• Trying to catch the panic, closing the client, and then resuming the unwind. So very, very many things do not like crossing a panic_unwind boundary, it wasn't even close to funny. I never got within about 10 errors of getting this to compile.
• Setting a panic hook to close the client. Oh my everything hated this idea. So many compile errors... (:thousand_yard_stare:)

It's entirely possible that any of the above approaches might work, in the hands of someone who knows what they're doing, but I haven't been able to get it work. Alternately, there may very well be dozens of other approaches that I just haven't thought of.

What I'm doing now is going completely panic-free in my tests, putting all the "interesting" code in a separate function that I pass the client, and having that function return a Result that's Err if anything went wrong or a condition wasn't met. The top-level test method then closes the client and panics on Err. This works, but it's fugly as all get out, and thoroughly unergonomic (all my hard-earned assert!() finger macros, just... gone).

In short... halp!

[ashwinsekaran]

+1 Following..

[jonhoo]

Thanks for the detailed write-up, and sorry for the late reply!

This is indeed a thing that sort of plagues the async ecosystem, and has, to my knowledge, no current good solutions. Asynchronous dropping is simply really annoying, and when paired with panics, it's even more of a pain.

Now, the good news is that tokio will catch panics from tasks and not shut down the runtime. And, since the client is just a channel sender: https://github.com/jonhoo/fantoccini/blob/bce77b0d1a871ce6dafc585e3045e244a79b6218/src/client.rs#L40

A panic in the thing that holds the client shouldn't automatically terminate the actual connection (which is held by the Session type). Instead, as long as the "real" connection future gets to keep running, it should hit this case and properly shut down: https://github.com/jonhoo/fantoccini/blob/bce77b0d1a871ce6dafc585e3045e244a79b6218/src/session.rs#L520-L527

However, I suspect what happens in your case is that you have something that boils down to this:

#[tokio::test]
async fn foo() {
    let client = /* ... */;
    assert!(false);
}

What happens here is that #[tokio::test] expands your test to roughly:

#[test]
fn foo() {
    tokio::runtime::Runtime::new()
      .block_on(async move {
          let client  = /* ... */;
          // real test code
          assert!(false);
      });

It uses block_on, which doesn't catch panics, and so a panic in the inner future panics the surrounding function. However, you could instead write it like this:

#[test]
fn foo() {
    tokio::runtime::Runtime::new()
      .block_on(async move {
          let client1  = /* ... */;
          let client = client1.clone();
          let r = tokio::spawn(async move {
              // real test code
              assert!(false);
           }).await;
          client1.close().await;
          if let Err(e) = r {
              std::panic::resume_unwind(e);
          }
      });

That way, even if the real test code panics, you'll make sure you close the session. You could probably wrap this in a little nice macro so that you don't have to repeat it for every test.

Alternatively, you may be able to utilize the unhandled_panic = "ignore" argument to #[tokio::test] which will ensure that even if the test does panic, the test code keeps running. I haven't used it myself though, so not entirely sure how the test does end up shutting down in that case, but some experimentation may tell you pretty quickly!

Best of luck!

[jonhoo]

Oh, and cargo-expand can be quite helpful for exploring things like #[tokio::test]!

[jonhoo]

Another option is to make use of something like what tokio uses to catch the panics from futures under the hood:

async fn run_with_client<F, Fut, T>(f: F) -> T
where
    F: FnOnce(&mut Client) -> Fut,
    Fut: Future<Output = T>,
{
    let mut client = Client;

    let res = futures::future::FutureExt::catch_unwind(AssertUnwindSafe(f(&mut client))).await;

    client.close().await;
    
    match res {
        Ok(t) => t,
        Err(panic) => std::panic::resume_unwind(panic),
    }
}