added test run time limits #890
Conversation
There was a problem hiding this comment.
This looks great!
- Please split this into two separate PRs, that is move everything "per item" into a new "draft" PR (or keep them locally for now, whatever works best for you). Let's then get the command line option merged first and do "per item" as a follow up.
- This needs a tests! Maybe just a single acceptance test will be fine, look at https://github.com/hspec/hspec/blob/main/hspec-core/test/Test/Hspec/Core/RunnerSpec.hs
NB: Things like timeouts are tricky to test, as we don't want to unnecessarily increase the runtime of the test suit. Still, maybe a timeout value of zero will do the trick, or maybe we will remove the test again later.
BebeSparkelSparkel
force-pushed
the
timelimits
branch
2 times, most recently
from
d57644d
to
764c6e8
Compare
|
I am unsure how my changes caused api-check / api-check (pull_request) to fail |
This is due to GHC 9.10.1. I fixed that with #892, I also added a section to the README on how to update API dumps. You will need to run that. The purpose of keeping track of API changes explicitly is so that we don't expose stuff that we didn't meant to, and to make it easier to determine whether we need a major or minor version bump. |
There was a problem hiding this comment.
Hey, this looks great. I left a couple of more comments. I haven't looked at a test case in detail yet, but from a quick glimpse, I assume this doesn't add any delays to the test suite, which is nice. Please take a look at my comments and make sure CI is green, then I will take a final look.
| config <- getArgs >>= readConfig c | ||
| oldFailureReport <- readFailureReportOnRerun config | ||
|
|
||
| let | ||
| forest :: [SpecTree ()] | ||
| forest = maybe id (fmap . fmap . setItemMaxTime) (configMaxTimePerTest config) f |
There was a problem hiding this comment.
I think this is not strictly needed and can be deferred to #891.
| evalItemDescription = requirement | ||
| , evalItemLocation = loc | ||
| , evalItemConcurrency = if isParallelizable == Just True then Concurrent else Sequential | ||
| , evalItemAction = \ progress -> measure $ e params withUnit progress | ||
| , evalItemAction = \ progress -> fmap (fmap $ fromMaybe $ Result requirement $ Failure loc $ Reason "exceeded timeout") $ measureWithTimeout maxTime $ e params withUnit progress |
There was a problem hiding this comment.
As I understand it, you could use maxTime instead of configMaxTimePerTest config here, for now.
Also, that line of code is getting quite long. I think it could make sense to extract some of the code into a separate function, say applyTimeout or failOnTimeout or both, not sure exactly how it will turn out nice, maybe just try something.
There was a problem hiding this comment.
As I understand it, you could use maxTime instead of configMaxTimePerTest config here, for now.
Sorry, I don't understand this.
| @@ -135,6 +143,7 @@ specItem s e = Leaf Item { | |||
| , itemIsParallelizable = Nothing | |||
| , itemIsFocused = False | |||
| , itemExample = safeEvaluateExample e | |||
| , itemMaxTime = Nothing | |||
There was a problem hiding this comment.
Same here, I think this can be deferred to #891.
There was a problem hiding this comment.
I am unsure where to get the timeout duration from if this is removed.
|
I'm not sure how to accommodate the unresolved. Also, if #891 is to be pulled then not storing the timeout in Item is extra work that will be undone. |
|
@BebeSparkelSparkel you marked a couple of my comments as resolved, but I don't see any associated code changes. Did you forget to push? |
|
Changed the wrong branch. I'll try and git fix it. |
BebeSparkelSparkel
force-pushed
the
timelimits
branch
from
4038f80
to
a40873e
Compare
|
I finalized an extension API that allows additional functionality to be implemented outside of Originally, I didn't want to delay your feature until the API is finalized, but now that I consider it done I think the way forward is to implement this feature as a separate package. This also means that you have complete freedom on how to name things, coding style, etc + you are not blocked by waiting for a code review from me. @BebeSparkelSparkel there are two options here, either you implement your feature with the git version of Once your package is on Hackage, I will then add it to Hspec's documentation, so that it can be used by others. |
|
@BebeSparkelSparkel the PR description contains an example on how to use the API + here is an other example: https://github.com/hspec/hspec/blob/ec2c16712ef68ba41d6f721d32840afadcba9576/hspec-core/src/Test/Hspec/Tags.hs |
|
I will try the git version implementation and add comment to the PR if I have issues. Since you were considering adding this to core, should I make this a new package in this repo since this is a fundamental part of testing? |
I suggest a separate package in a separate repo. We can always decide to upstream things later. I could imagine to upstream By the way, if you think a separate repo is too much overhead, you could add it to |
|
Ok, do extensions also allow for the implementation of |
Yes, I think so. Look at the "tags" example. |
@BebeSparkelSparkel I think even the "ci" example should demonstrate all the ideas, but it's much shorter. So maybe that's a better place to start. Ask if you need any specific advice or help. |
|
Thank you I will attempt this now |
Allow tests to have limited run time via a command line argument or Spec modifiers.
closes #889