-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Fix the clock resolution to millis in ScheduledEventTests #38506
Fix the clock resolution to millis in ScheduledEventTests #38506
Conversation
The clock resolution changed from jdk8 to jdk10, hence the test is passing in jdk8 but failing in jdk10. Scheduled events are serialised and deserialised with millisecond precision, making test fail in jdk 10 and higher. Fixes a problem introduced by elastic#38415 and the fix is identical to the one that was made in elastic#38405.
Pinging @elastic/ml-core |
@@ -119,4 +120,8 @@ public void testLenientParser() throws IOException { | |||
ScheduledEvent.LENIENT_PARSER.apply(parser, null); | |||
} | |||
} | |||
|
|||
private static ZonedDateTime nowWithMillisResolution() { |
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.
Should we factor this out into a base class now it's used in two places (the other is GetWatchResponseTests
)? ESTestCase
? What do you think @spinscale?
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 very tempting as x-pack/sql is already doing the same (in a slightly different way)
#36877
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.
LGTM
we might want to refactor the nowWithMillisResolution
in some common place - DateUtils? But I guess better to do this in a separate PR touching watcher, sql and ML (will create an issue to remember about this)
Yes, agreed, especially now this PR has passed all checks. I will merge it now to minimise build failures. |
) The clock resolution changed from jdk8 to jdk10, hence the test is passing in jdk8 but failing in jdk10. Scheduled events are serialised and deserialised with millisecond precision, making test fail in jdk 10 and higher. Fixes a problem introduced by elastic#38415 and the fix is identical to the one that was made in elastic#38405.
) The clock resolution changed from jdk8 to jdk10, hence the test is passing in jdk8 but failing in jdk10. Scheduled events are serialised and deserialised with millisecond precision, making test fail in jdk 10 and higher. Fixes a problem introduced by elastic#38415 and the fix is identical to the one that was made in elastic#38405.
* master: ML: update set_upgrade_mode, add logging (elastic#38372) bad formatted JSON object (elastic#38515) (elastic#38525) Fix HistoryIntegrationTests timestamp comparsion (elastic#38505) SQL: Fix issue with IN not resolving to underlying keyword field (elastic#38440) Fix the clock resolution to millis in ScheduledEventTests (elastic#38506)
) The clock resolution changed from jdk8 to jdk10, hence the test is passing in jdk8 but failing in jdk10. Scheduled events are serialised and deserialised with millisecond precision, making test fail in jdk 10 and higher. Fixes a problem introduced by elastic#38415 and the fix is identical to the one that was made in elastic#38405.
The clock resolution changed from jdk8 to jdk10, hence the test is passing in jdk8 but failing in jdk10. Scheduled events are serialised and deserialised with millisecond precision, making test fail in jdk 10 and higher. Fixes a problem introduced by #38415 and the fix is identical to the one that was made in #38405. Backport of #38506
* master: (27 commits) Mute AnalysisModuleTests#testStandardFilterBWC (elastic#38636) add geotile_grid ref to asciidoc (elastic#38632) Enable Dockerfile from artifacts.elastic.co (elastic#38552) Mute FollowerFailOverIT testFailOverOnFollower (elastic#38634) Account for a possible rolled over file while reading the audit log file (elastic#34909) Mute failure in InternalEngineTests (elastic#38622) Fix Issue with Concurrent Snapshot Init + Delete (elastic#38518) Refactor ZonedDateTime.now in millis resolution (elastic#38577) Mute failing WatchStatusIntegrationTests (elastic#38621) Mute failing ApiKeyIntegTests (elastic#38614) [DOCS] Add warning about bypassing ML PUT APIs (elastic#38509) Add 7.1 and 8.0 version constants to master (elastic#38514) ML: update set_upgrade_mode, add logging (elastic#38372) bad formatted JSON object (elastic#38515) (elastic#38525) Fix HistoryIntegrationTests timestamp comparsion (elastic#38505) SQL: Fix issue with IN not resolving to underlying keyword field (elastic#38440) Fix the clock resolution to millis in ScheduledEventTests (elastic#38506) Enable BWC after backport recovering leases (elastic#38485) Collapse retention lease integration tests (elastic#38483) TransportVerifyShardBeforeCloseAction should force a flush (elastic#38401) ...
The clock resolution changed from jdk8 to jdk10, hence the
test is passing in jdk8 but failing in jdk10. Scheduled
events are serialised and deserialised with millisecond
precision, making test fail in jdk 10 and higher.
Fixes a problem introduced by #38415 and the fix is
identical to the one that was made in #38405.