-
Notifications
You must be signed in to change notification settings - Fork 109
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
Tasty progress reporting #311
Tasty progress reporting #311
Conversation
8bbea9e
to
ddc9e38
Compare
That's so cool! I'll try to take a look this week. |
Thanks again for doing this. FYI, I rebased your branch and added some changes at Unfortunately, with this implementation the performance seems to suffer. As a benchmark, I used the example program from the readme, which I run with Here's what I observe:
This is especially acute since long-running tests are exactly those where you want to see the progress but also care about how long they take. (This is after I fixed a spinning loop — the original slowdown was over 2x.) In order for But even with progress, we should try to optimize this, as this will be the default behavior. It'd be also interesting to measure whether raw QuickCheck itself has a comparable slowdown from printing the progress. |
I don't see much difference when I remove |
I was comparing to a wrong commit, I can reproduce similar results. |
I made a profile build of the master
try-adding-progress-rpt
try-adding-progress-rpt --no-progress
|
ddc9e38
to
7450612
Compare
From the flamegraphs: it's clear that |
7450612
to
b81d877
Compare
Moving the no progress check to |
25f5e36
to
035bf29
Compare
035bf29
to
ce6a2e5
Compare
@Bodigrim I addressed your review remarks. I left a few unresolved, please check my comments. |
ce6a2e5
to
ad6d91b
Compare
ad6d91b
to
749c4a7
Compare
@coot I appreciate that this was a long haul, and you demonstrated incredible amount of patience. Do you think you might get time to finish this up soon? Or may I take over? I'm looking to make a release of |
I can address outstanding comments this week. |
ec5d7b5
to
1db3408
Compare
@Bodigrim I addressed your review remarks. |
1db3408
to
5e17fb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there!
CC @martijnbastiaan for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool! I've got some minor suggestions.
Unless there are any substantial objections, I'm going to merge this somewhere next week as is and fix outstanding issues in a subsequent PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed CI after breaking it with modern haddock (needs haddock for GHC 8.6 or higher).
I suggest a short migration instruction for PrintTest
(see comment).
There are bits that are still not very clever but it seems to work. I'm concerned about a performance hit from constantly thrashing the TVars and checking them all the time.
QuickCheck has its own progress information which shows number of test cases done so far and if a test fails also reports the number of shrinks. Tasty disables QuickCheck's output by setting 'chatty' option to 'False'. We can now feed this information to tasty progress reporting by defining how 'QuickCheck's speaks to a terminal.
Without these changes, I get multiple dots from QuickCheck on the terminal with --quickcheck-tests=1000000 Either of them suffices for a fix, but they both make sense.
And bumped version to 0.10.3.
This haddock is too old for parsing constructor field documentation.
cfbd282
to
29f43a4
Compare
Thanks @coot and @mankyKitty! |
import qualified Test.QuickCheck.Test as QC | ||
import qualified Test.QuickCheck.State as QC | ||
import qualified Test.QuickCheck.Text as QC | ||
import Test.Tasty.Runners (formatMessage, emptyProgress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires tasty >= 1.5
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is reflected in the tasty-quickcheck.cabal
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the test-suite
, yes, but if I understood #386 correctly it is also needed for the library
.
This PR is an updated and cleaned version of #245.
The upstream version introduced a bug which prevented quickcheck properties to
be shrinked. The approach in this PR avoids that by using QuickCheck
facilities to print its own progress using tasty progress callback.