tasty icon indicating copy to clipboard operation
tasty copied to clipboard

Progress reporting for QuickCheck seems to report progress over 100%

Open michaelpj opened this issue 1 year ago • 10 comments

I'm not really sure what's going on here, but I've seen progress reports from QuickCheck go over 100%. These are for tests where I've set MaxSuccesses to a higher number, so initially I thought we simply weren't dividing by the right thing. But the code seems to be doing the right thing so I'm not sure what's happening.

cc @coot

michaelpj avatar Dec 04 '23 15:12 michaelpj

:thinking: I have never observed that with QuickCheck itself in ghci, have you?

coot avatar Dec 20 '23 09:12 coot

I don't think this is anything to do with QuickCheck - that just reports the number of tests, which seems fine. I think it must be something about how we're converting that to a percentage...

michaelpj avatar Dec 22 '23 21:12 michaelpj

Maybe tasty way to parse QuickCheck messages is not entirely correct. Any chance that the tests in question have high discard rate?

Bodigrim avatar Feb 21 '24 00:02 Bodigrim

Example of this problem

All
  Properties
    lowlevel
      input-output
        t_write_read:         384% 

when using withMaxSuccess 1000 ...

BebeSparkelSparkel avatar May 02 '24 18:05 BebeSparkelSparkel

Error in this line https://github.com/UnkindPartition/tasty/blob/dcbf32078133aa2b569774c417cc49a49f5c573b/quickcheck/Test/Tasty/QuickCheck.hs#L267

BebeSparkelSparkel avatar May 02 '24 18:05 BebeSparkelSparkel

@BebeSparkelSparkel any idea what's exactly wrong with parseProgress?

Bodigrim avatar May 02 '24 21:05 Bodigrim

@Bodigrim Nothing wrong with the parser. QuickCheck prints the number of tests run and not the % done. Also, the number of tests to run cannot be gotten with out running at least one test because withMaxSuccess does not modify the state nor args but rather the returned intermediate result that has the field maybeNumTests.

I was thinking that an IORef could be added that holds the number of tests to run and then mapResult could be used with unsafePerformIO to update the number after one tests has been run.

I also added this issue since QuickCheck seems to not be a primary test framework but rather an add in tool. https://github.com/nick8325/quickcheck/issues/399

BebeSparkelSparkel avatar May 04 '24 20:05 BebeSparkelSparkel

@BebeSparkelSparkel I'm afraid it's hard for me to grasp. Can you possibly come up with a minimal standalone reproducer?

Bodigrim avatar May 05 '24 11:05 Bodigrim

OK, so the problem is that there are two ways to set number of tests to execute.

One is to use QuickCheckTests on TestTree level, e. g., localOption (QuickCheckTests 1000). This is a native tasty-quickcheck mechanism, which progress reporting knows about and handles well.

But there is another way to set number of tests: apply withMaxSuccess on Property level. This way it remains hidden from tasty, which continues to think that there are only 100 tests, thus misreporting progress.

@BebeSparkelSparkel is my understanding correct?

Bodigrim avatar May 05 '24 16:05 Bodigrim

Yes. I really don't like how withMaxSuccess accomplishes this.

BebeSparkelSparkel avatar May 05 '24 17:05 BebeSparkelSparkel

You can always add a callback that uses an IORef to track the changes to Result that are made by withMaxSuccess and friends.

MaximilianAlgehed avatar May 28 '24 07:05 MaximilianAlgehed

@michaelpj could you please check whether HEAD fixes the issues you observed?

Bodigrim avatar Jun 09 '24 17:06 Bodigrim

Yep, that fixes it! Thanks @BebeSparkelSparkel

michaelpj avatar Jun 13 '24 10:06 michaelpj