Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Nov 3, 2019

What changes were proposed in this pull request?

  • Retry the tests for special date-time values on failure. The tests can potentially fail when reference values were taken before midnight and test code resolves special values after midnight. The retry can guarantees that the tests run during the same day.
  • Simplify getting of the current timestamp via Instant.now(). This should avoid any issues of converting current local datetime to an instance. For example, the same local time can be mapped to 2 instants when clocks are turned backward 1 hour on daylight saving date.
  • Extract common code to SQLHelper
  • Set the tested zoneId to the session time zone in DateTimeUtilsSuite.

Why are the changes needed?

To make the tests more stable.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By existing test suites Date/TimestampFormatterSuite and DateTimeUtilsSuite.

@MaxGekk MaxGekk changed the title [SPARK-29736][TEST] Improve stability of tests for special datetime values [SPARK-29736][TESTS] Improve stability of tests for special datetime values Nov 3, 2019
@SparkQA
Copy link

SparkQA commented Nov 3, 2019

Test build #113183 has finished for PR 26380 at commit 44aeda8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class DateTimeUtilsSuite extends SparkFunSuite with Matchers with SQLHelper

@MaxGekk
Copy link
Member Author

MaxGekk commented Nov 4, 2019

@HyukjinKwon @cloud-fan Could you take a look at this.

  1. This is rare case but could happen
  2. This happened once I guess in [SPARK-29733][TESTS] Fix wrong order of parameters passed to assertEquals #26377 (comment)

try {
test(zoneId)
} catch {
case NonFatal(_) => test(zoneId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we set a threshold like 3? otherwise we may hit dead loop for real failures.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I don't understand how such loop could happen but this is not recursive code. test can be invoke maximum 2 times.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I mis-looked. It's not recursive. LGTM

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 5053860 Nov 4, 2019
@MaxGekk MaxGekk deleted the retry-on-fail branch June 5, 2020 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants