UnkindPartition / tasty

Modern and extensible testing framework for Haskell
640 stars 110 forks source link

Output quickcheck seed on timeout #359

Closed brandonchinn178 closed 4 months ago

brandonchinn178 commented 1 year ago

We had a quickcheck test timeout (using tasty's --timeout option), but we can't repro it because we don't have the seed. If we had the seed, we could see what quickcheck generated and see if it generated a pathological input that we haven't handled.

One possible solution that doesn't break the public API:

  1. Pass the handle to the test thread (asy) to applyTimeout
  2. Throw a new TastyTimeout exception to the test thread if the test thread takes too long
  3. If asy returns a result within 1 second (or some other short time), return that result
  4. Otherwise, do the current behavior: cancel asy and return a generic "Timed out" result
  5. Update tasty-quickcheck's run implementation to catch TastyTimeout and return a Result with the seed added to the description
    • Break out the current timeoutResult definition into mkTimeoutResult :: TastyTimeout -> Result

A possible solution that changes the IsTest interface (I know maintainers don't want to make breaking changes, but I think this would make tasty more flexible that can enable other future improvements):


Old solution that doesn't work 1. Add a new `TastyTimeout` exception containing information of the timeout, which should be thrown instead of the exception thrown by `timeout` 2. Break out `timeoutResult` into `mkTimeoutResult :: TastyTimeout -> Result` 7. Wrap the `tasty-quickcheck` [runner](https://github.com/UnkindPartition/tasty/blob/7379d3101f165a582aa3341084fbeaae9828875e/quickcheck/Test/Tasty/QuickCheck.hs#L214-L227) with ```hs handle (pure . mkTimeoutResultWithSeed replaySeed) $ do ... where -- N.B. technically violates the rule to always rethrow async exceptions, -- but i think this exception to the rule makes sense mkTimeoutResultWithSeed seed e = let result = mkTimeoutResult e in result{resultDescription = resultDescription result ++ printf " -- with seed: %d" seed} ``` This doesn't work because the test is run in a separate thread from the driver, and the timeout exception is handled in the driver thread and doesn't make it to the test thread. The test thread does get the `AsyncCancelled` exception thrown to it, but the driver thread never looks at the result of the test thread after the timeout, so it'll just get thrown away.
Bodigrim commented 1 year ago

What about passing timeout to QuickCheck via within? Seems least invasive solution to me.

brandonchinn178 commented 1 year ago

@Bodigrim I'm not sure that applies to this issue. The goal isn't to set a timeout, the goal is for tasty-quickcheck to register an action that runs when tasty times out.

Bodigrim commented 1 year ago

We had a quickcheck test timeout (using tasty's --timeout option), but we can't repro it because we don't have the seed. If we had the seed, we could see what quickcheck generated and see if it generated a pathological input that we haven't handled.

If you use within, QuickCheck is supposed to display you a pathological input, omitting any need to figure out the seed and such.

brandonchinn178 commented 1 year ago

Are you suggesting that we should add within to every test that can timeout, with a timeout shorter than the timeout we tell tasty? I guess that's possible, but it'd be nice for tasty-quickcheck to automatically register this.

Although maybe tasty-quickcheck can automatically find the current timeout and add within to the property?

Bodigrim commented 1 year ago

Although maybe tasty-quickcheck can automatically find the current timeout and add within to the property?

Yes, this should be possible.

Bodigrim commented 7 months ago

It appears that Test.QuickCheck.within talks about timeouts for individual QuickCheck tests, while Tasty timeout limits entire duration of a property (which typically is 100 tests or more, depending on QuickCheckTests).

Also, applying within atop of a property does not print a counterexample because of https://github.com/nick8325/quickcheck/issues/250. But it prints --quickcheck-replay argument, so one can put a trace and run again.

All in all, we can add a new command line argument --quickcheck-timeout and translate it into within wrapper.