-
Notifications
You must be signed in to change notification settings - Fork 719
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
Make UTCTime test generator generate values without leap seconds #5198
Conversation
My concern is that this is not how the Cardano system interprets time. We made the decision to stick with UTC (and live with the issues of leap seconds) rather than some form of strict montonic clock (such as TAI or some offset from it). We are using UTC “on chain” and we should not use other clocks in testing - otherwise other issues will creep in like TTLs not working as expected and/or nodes creating blocks early/late as leap second effect accumulate We just have to accept (and recognise as false positives) related test failures. So this is not way that this issue can be resolved - sorry to be bearer of bad news |
I wanted to stop this being merged (see comment above) - I'm not a code owner, so I converted it to a DRAFT so we can discuss this before it ends up in master (which, as you can see from my comment above) I don't think it should @disassembler @Jimbo4350 |
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 can't see that this has anything to do with leap seconds, as the PR description says.
Can you clarify?
As far as I can see it's just replacing a bogus generator with a sane one.
LGTM Note this generator is used to generate a time to do a round trip serialisation/deserialisation test and not for any timing related tests and we could not use this generator for timing related tests anyway because it generates values for all possible times and we would not be able to do timing tests with values from such a large range. |
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.
GIven that we've resolved this not to be about UTC v other time models but one of an incorrect generator of values, now made 'sane'. LGTM
I've updated the description with your remarks.
This generator is only using for round-trip json serialization/deserialization, so it shouldn't affect the rest of the compontents.
The range (1E9; 2E9) is used to represent a leap second. The values above 2E9 would indicate more than one second, which is not in the UTC standard, which results in the test failure, because deserialization relies on the fact that there could be only one leap second. |
Description
This PR changes
UTCTime
generator, which is only used ingenKesPeriodInfoOutput
generator for round-trip testing of JSON decoding/encoding of theQueryKesPeriodInfoOutput
type.Previous
UTCTime
generator was usingSystemTime
and was generating values in the range[0; 2^32-1]
for representing nanoseconds part of the time. The maximum allowed value of nanoseconds inSystemTime
is2E9-1
, where values from range(1E9;2E9-1]
are used to represent leap seconds. Therefore, the previous generator was able to generate values above2E9-1
, introducing this way extra leap seconds (up to 4), where UTC standard supports only one leap second (see ITU recommendation for UTC).Unfortunately, in the chain of functions
generate Word32 -> create SystemTime -> convert to UTCTime -> encode into JSON -> decode from JSON
this leap seconds condition is checked only at the JSON decoding stage, which results in a test failure there.Observed failure: https://ci.zw3rk.com/build/2171231/nixlog/2
This fix changes the generation method to use unix time to create a valid UTC timestamp.
See also: haskell/time#242
Checklist
See Runnings tests for more details
CHANGELOG.md
for affected package.cabal
files are updatedhlint
. See.github/workflows/check-hlint.yml
to get thehlint
versionstylish-haskell
. See.github/workflows/stylish-haskell.yml
to get thestylish-haskell
versionghc-8.10.7
andghc-9.2.7