tz-magic when dst-transition is near #561

FSchumacher · 2020-03-08T16:42:04Z

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:

My code follows the code style of this project.

vlsi · 2020-03-08T16:43:39Z

src/functions/src/test/java/org/apache/jmeter/functions/TestTimeShiftFunction.java

@@ -106,13 +109,35 @@ public void testNowPlusOneDay() throws Exception {
        assertThat(tomorrowFromFunction, sameDay(tomorrow));
    }

+    private int dstOffset(Date from, Date to) {


Why is this required?

Because it is called from line 139?
But seriously that function is the essence of this PR, as it calculates the DST offset between two dates for the timezone of the JVM.

vlsi · 2020-03-08T16:44:47Z

@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 · 2020-03-08T17:00:22Z

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 · 2020-03-08T17:15:24Z

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?

It is not clear what it is doing
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 · 2020-03-08T17:26:54Z

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 · 2020-03-08T19:21:19Z

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 · 2020-03-08T20:27:03Z

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 · 2020-10-07T20:41:54Z

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

FSchumacher · 2020-10-10T11:35:49Z

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).

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.

codecov-io · 2021-03-13T14:19:17Z

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 · 2021-09-18T11:13:50Z

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

vlsi reviewed Mar 8, 2020

View reviewed changes

FSchumacher force-pushed the tz-magic branch from e804d1e to 1ff1091 Compare March 13, 2021 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tz-magic when dst-transition is near #561

tz-magic when dst-transition is near #561

FSchumacher commented Mar 8, 2020

vlsi Mar 8, 2020

FSchumacher Mar 8, 2020

vlsi commented Mar 8, 2020

FSchumacher commented Mar 8, 2020 •

edited

Loading

vlsi commented Mar 8, 2020

FSchumacher commented Mar 8, 2020

vlsi commented Mar 8, 2020

FSchumacher commented Mar 8, 2020

pmouawad commented Oct 7, 2020

FSchumacher commented Oct 10, 2020

codecov-io commented Mar 13, 2021

FSchumacher commented Sep 18, 2021

tz-magic when dst-transition is near #561

Are you sure you want to change the base?

tz-magic when dst-transition is near #561

Conversation

FSchumacher commented Mar 8, 2020

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

vlsi Mar 8, 2020

Choose a reason for hiding this comment

FSchumacher Mar 8, 2020

Choose a reason for hiding this comment

vlsi commented Mar 8, 2020

FSchumacher commented Mar 8, 2020 • edited Loading

vlsi commented Mar 8, 2020

FSchumacher commented Mar 8, 2020

vlsi commented Mar 8, 2020

FSchumacher commented Mar 8, 2020

pmouawad commented Oct 7, 2020

FSchumacher commented Oct 10, 2020

codecov-io commented Mar 13, 2021

Codecov Report

FSchumacher commented Sep 18, 2021

FSchumacher commented Mar 8, 2020 •

edited

Loading