tasty

tasty copied to clipboard

Uses a TVar to store the latest progress information.

Closes #243

I added a couple of record types to the StatusMap creation process as it was becoming a bit gnarly with the progress TVar mixed in. I can revert that or change it up if there are strong feelings about it. It made it a bit easier for me to process what was going on with the extra types. :)

There are bits that are still not very clever but it seems to work...

Bits I think need work:

• [x] Formatting of the progress message needs to account for more use cases:
  • Non-empty text, zero pct
  • empty text, non-zero pct
  • both non-zero
• [x] ~Consider changing input type to yieldProgress to Maybe Progress or something to let the downstream user decide when/if to report progress.~ Not really needed and would break things unnecessarily, I think.
• [x] The ppProgressOrStatus function "works", but I'm confident there is a better way to do that.

Still keen for more feedback but I think this in a good spot now.

There shouldn't be any API disruption unless packages had busted progress reporting that will suddenly come alive.

I struggled to get your changes into my branch, I ended up rebasing them in and ensuring I was still in the correct spot, so everything should be there! I was probably over thinking it and should have just had a merge commit or something.

Apologies, I'm happy to kick off again on a new PR branch if this is going to make your life difficult or the repo history weird.

I'll work on adding those features from your list:

• [x] Don't print progress if you're not in a terminal.
• [x] Add option to disable progress reporting.
• [x] Add a delay to avoid spamming progress updates and slowing things down

I struggled to get your changes into my branch, I ended up rebasing them in and ensuring I was still in the correct spot, so everything should be there!

Hmm, that doesn't sound right. My branch contained all of your changes, so you could just git reset --hard to my branch. There was no need to rebase anything on your part; I did all the rebasing precisely to make your life easier :)

So to make it clear, what I'd like you to do is (assuming you have no new changes):

git checkout try-adding-progress-rpt
git reset --hard feuerbach/try-adding-progress-rpt # or whatever is my remote called on your side

and then commit your future changes simply on top of this branch.

If you do have new changes, you can do git rebase --onto. Let me know if you'd like some instructions.

I understood why you put in the effort to rebase the changes, but I didn't know how to properly leverage it and my doc diving/searching lead me to that solution. I didn't clue into using git reset with a reference, but thinking about the graph I can see how that makes sense.

It's entirely PEBKAC on my behalf! :)

I've also learned a bunch about rebasing and merging, so that's neat. :+1:

I've run those commands and everything appears to be happy, I can still see your changes in the log and the history isn't doing anything hinky.

Do you think the delay for progress reporting should be configurable ? I'm not sure it makes much sense, but someone might find it a useful knob to fiddle with?

The options for hiding progress, obeying the --quiet option, and only printing in the right sort of terminal should be working now. :)

Is there anything left to be done for this ? I don't want it to drift, more than it already has :<

Sorry, I just haven't had the bandwidth to take care of this so far. I hope to find the time within a month or two, although I can't make any promises.

Cool, sorry was more worried about me having left things in a broken/unwanted state. :)

This looks awesome! Can I help to finish this one off?

Thanks for the offer @alexbiehl. The task list would be roughly as follows:

• [ ] do a general code review
• [ ] test (by hand) that it works as expected
• [ ] check that any new functions/types are documented in the haddocks, and any new user-facing changes are documented in the README. (No need to document the reporting itself, but things like new options should be documented.)
• [ ] check for the things described here https://ro-che.info/articles/2016-09-18-good-pull-requests; in particular, squash the commits as appropriate
• [ ] rebase on top of the current master
• [ ] figure out the correct version bump, update the cabal & change log files

If you can tick these off, I'll be happy to merge. I guess you won't be able to update the PR; just let me know the repo/branch and I'll pull from there, no need to create a new PR.

I pushed an updated branch here. However, the approach for quickcheck is wrong: by wrapping any property shrinking will be discarded: if one has a property prop :: Int -> Bool then the property: QuickCheck.monadicIO $ QuickCheck.run (return prop) :: Property and hence it will not shrink.

@coot thanks, good catch, we definitely don't want that. I guess we can always go through unsafePerformIO; are there any better ideas? Might ask QuickCheck devs what they think.

@UnkindPartition

• I found a way to report quickcheck status through tasty yieldProgress (no need for unsafePerformIO, which anyway wouldn't work, since top level Property is only evaluated once). It reports exactly what quickcheck's chatty option does, e.g. it reports number of tests done so far (rather than a %) and once a test fails it will also report the number of shrinks done so far as well.
• hunit does not use progress, so it's ok
• smallcheck - I am not familiar with it; but it seems to me that smallcheck does not shrink test cases so its progress reporting can be simpler that quickcheck's.

Should I open a new PR? Do you have time to review it?

Lack of progress reporting (especially for QC tests) is one of the reasons why we haven't made the transition from test-framework to tasty yet. So I am interested to see this finished. Can I help? What is the current status?

What is the current status?

If someone makes a mergeable PR (this one has conflicts), the current team of maintainers might make an attempt to review. This is a valuable feature and I lack it myself, but we do not have much bandwidth dedicated to tasty and are overwhelmed with other projects, so review is likely to take quite a while.

@Bodigrim I updated my pr and fixed a performance regression: #311.

Closing in favor of #311.