tz-magic when dst-transition is near

[FSchumacher]

Description

Calculate the DST offset in a special scenario for timeshift function tests, that cross DST switching.

Motivation and Context

Our tests (for timeshift function) started to fail shortly before a DST transition in New Yorker timezone, as the parsing of a date into LocalDateTime seems to follow different rules than adding time to a LocalDateTime instance.

Therefore we now try to calculate the DST offset for the calculated time and the current time. That offset will be added to the result of the timeshift function.

How Has This Been Tested?

A test VM with a date set to 2020-03-04 has been set up and used to run ./gradlew build.

Screenshots (if appropriate):

Types of changes

• Fix of a test

Checklist:

• [x] My code follows the code style of this project.

[vlsi]

@FSchumacher , can you please clarify what behavior you want to achieve before adding dst-like code?

In 99.42% of the cases, dstOffset code is invalid, and it should be avoided.

[FSchumacher]

In normal times (no DST switch in the near future) the dstOffset method should return 0.

Can you tell me, where dstOffset is wrong? My tests seemed to be correct (without the patch the test fails and with the patch it passes).

But I have to admit, that I don't feel comfortable with this hack, so if you have a better version that works in the interval 10 days before a DST switch in the local timezone, I would be happy. And how did you come up with 99.42 %?

PS. the reason for this should be clear from the description of the PR above.

[vlsi]

if you have a better version that works

What do you mean by works ?

What is the intended behavior when "adding 1 day"? Do you intend it always to match "adding 24 hours"?

Can you tell me, where dstOffset is wrong?

1. It is not clear what it is doing
2. It is likely to duplicate existing java.time. method

I think we should stick to java.time. APIs unless it is clear java.time. is not enough.

[FSchumacher]

if you have a better version that works

What do you mean by works ?

The tests should pass all the time of the year. Currently they fail for about 10 days before a DST switch in the current timezone of the JVM. (At least on some instances of travis and my test vm, that I had setup for testing).

What is the intended behavior when "adding 1 day"? Do you intend it always to match "adding 24 hours"?

Where do you read from the code, that I add 1 day or 24 hours?

The current test for timeshift uses the current date for the local timezone of the machine and adds a specified (complex) unit of time to it (that time is then parsed back from a string representation of it). The test then checks, that it is the same instant as the local current date with the same amount of time added via a different API.

This seems to break when we cross the DST setting (DST switch somewhere between now and the future date).

Can you tell me, where dstOffset is wrong?

1. It is not clear what it is doing

That can hopefully fixed once I understand how to describe it properly :)

2. It is likely to duplicate existing `java.time.` method

Is there a java.time. function that gives the offset for DST between different dates?

I think we should stick to java.time. APIs unless it is clear java.time. is not enough.

+1 to that

[vlsi]

Where do you read from the code, that I add 1 day or 24 hours?

Ok. TimeShift function is using java.time.Duration, thus it always treats "day" unit as 24 hours (see javadoc for java.time.Duration vs java.time.Period)

The test code is using LocalDateTime which has absolutely no relation to time zone.

Does it really make sense to use LocalDateTime in the test code? Should the test be using ZonedDateTime instead?

[FSchumacher]

Where do you read from the code, that I add 1 day or 24 hours?

Ok. TimeShift function is using java.time.Duration, thus it always treats "day" unit as 24 hours (see javadoc for java.time.Duration vs java.time.Period)

The test code is using LocalDateTime which has absolutely no relation to time zone.

Does it really make sense to use LocalDateTime in the test code? Should the test be using ZonedDateTime instead?

I think I now understand where the problems come from. We changed to ZonedDateTime inside of timeshift a while ago. The idea was to support time zones, which is a good thing.

Now the test method we are talking about uses a format string which has no time zone attribute and thus the timeshift function will format the shifted time to a string that has no DST correct time zone information. When we parse that back, it has an offset to the one we calculated by hand.

Now, if we try to change to ZonedDateTime inside of the test method, we would probably gain not really much, as the important part – the time offset – is missing in the formatted time string.

I will try, if we can get that missing information by looking at the future date alone.

[pmouawad]

Hello @FSchumacher , Is this PR still valid ? Why not merge it ? Thanks

[FSchumacher]

The reasons for this PR are still in place. We handle timezones not really correctly and therefore get flaky tests shortly before and after the dates for winter/summertime adjustments. We have two ways of handling this. First, we could try to correct the way we handle timezones in the JMeter function. (I didn't get far with that and gave up). Second, we could handle the time offset in the test case, making sure, that it doesn't fail in that short period before and after the date of time adjustment (which is, what has been done in this PR, but Vladimir seems to dislike it and I don't want to merge stuff, that he dislikes).

[codecov-io]

Codecov Report

Merging #561 (1ff1091) into master (66ae8f0) will decrease coverage by 0.00%. The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #561      +/-   ##
============================================
- Coverage     55.39%   55.39%   -0.01%     
+ Complexity    10191    10189       -2     
============================================
  Files          1046     1046              
  Lines         64356    64356              
  Branches       7299     7299              
============================================
- Hits          35651    35649       -2     
  Misses        26196    26196              
- Partials       2509     2511       +2     

Impacted Files	Coverage Δ	Complexity Δ
...ache/jmeter/reporters/SummariserRunningSample.java	83.58% <0.00%> (-1.50%)	18.00% <0.00%> (-1.00%)
...n/java/org/apache/jmeter/reporters/Summariser.java	84.73% <0.00%> (-0.77%)	17.00% <0.00%> (-1.00%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 66ae8f0...1ff1091. Read the comment docs.

[FSchumacher]

More discussion on can be found on https://bz.apache.org/bugzilla/show_bug.cgi?id=65217