Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Output quickcheck seed on timeout #359

Open
brandonchinn178 opened this issue Dec 12, 2022 · 6 comments
Open

Output quickcheck seed on timeout #359

brandonchinn178 opened this issue Dec 12, 2022 · 6 comments

Comments

@brandonchinn178
Copy link
Contributor

brandonchinn178 commented Dec 12, 2022

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):

  • Add a new TestMeta type containing hooks for test providers; currently, it would contain a single field: resultOnTimeout :: Maybe Result
    • Pass an IORef TestMeta parameter to run that run can modify as it executes
    • Change run :: test -> IO Result to run :: test -> IO (TestMeta, IO Result)

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

  3. Wrap the tasty-quickcheck runner with

    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
Copy link
Collaborator

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

@brandonchinn178
Copy link
Contributor Author

@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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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

Yes, this should be possible.

@Bodigrim
Copy link
Collaborator

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 nick8325/quickcheck#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants