-
Notifications
You must be signed in to change notification settings - Fork 846
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
feat: provide time indication for integration tests #6615
Conversation
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.
Subject to some minor observations, looks good to me.
tests/integration/IntegrationSpec.hs
Outdated
let (successes, failures) = partition ((== ExitSuccess) . snd) | ||
$ Map.toList results | ||
|
||
let timeDiff = diffUTCTime finalTime startTime | ||
let timeDiffStr = formatTime defaultTimeLocale "%h:%m:%s" timeDiff |
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.
Did you mean "%h:%m:%s"
, or was your intention "%H:%M:%S"
. The former generates output like:
2024-06-22T16:28:37.8924296Z Integration tests ran in : 0:19:1191
because 1,191 seconds are equivalent to about 19 whole minutes and about 0 whole hours.
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 switched to "%H:%M:%S - total %s seconds"
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 misunderstood the docs)
tests/integration/IntegrationSpec.hs
Outdated
@@ -60,12 +62,14 @@ main = runSimpleApp $ do | |||
<> fromString (takeFileName next) | |||
res <- test next | |||
loop (idx + 1) rest' (res <> accum) | |||
|
|||
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.
Some redundant spaces have slipped in.
tests/integration/IntegrationSpec.hs
Outdated
@@ -31,6 +31,7 @@ import RIO.Process | |||
) | |||
import qualified RIO.Set as Set | |||
import qualified RIO.Text as T | |||
import RIO.Time ( getCurrentTime, diffUTCTime, formatTime, defaultTimeLocale ) |
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.
If you happen to make other revisions (see below), it would be good to list the imports in alphabetical order, for consistency.
tests/integration/IntegrationSpec.hs
Outdated
|
||
let timeDiff = diffUTCTime finalTime startTime | ||
let timeDiffStr = formatTime defaultTimeLocale "%h:%m:%s" timeDiff | ||
logInfo (fromString ("Integration tests ran in : " ++ timeDiffStr)) |
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.
The OverloadedStrings
means that you do not have to apply the fromString
to the literal:
logInfo $ "Integration tests ran in : " <> fromString timeDiffStr
tests/integration/IntegrationSpec.hs
Outdated
@@ -77,6 +81,7 @@ main = runSimpleApp $ do | |||
logInfo "Failed tests:" | |||
for_ failures $ \(x, ec) -> logInfo $ "- " <> display x <> " - " <> displayShow ec | |||
exitFailure | |||
|
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.
Some redundant blank lines have slipped in.
@mpilgrem fixed the issues |
Provides time indication for integration tests.
It's obviously not a very good benchmark, but it's still better than nothing for very big variations.
For posterity :
1191; 1237 - Ubuntu
980; 1018 - Macos latest
1578; 1515 - Windows latest