Bug 65217 - Problem in test for timeShift function
Summary: Problem in test for timeShift function
Status: NEEDINFO
Alias: None
Product: JMeter
Classification: Unclassified
Component: Main (show other bugs)
Version: 5.4.1
Hardware: PC All
: P2 minor (vote)
Target Milestone: JMETER_5.5
Assignee: JMeter issues mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-04-03 21:41 UTC by Mariusz_W
Modified: 2021-06-08 06:28 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mariusz_W 2021-04-03 21:41:20 UTC
It seems that TestTimeShiftFunction.testNowWithComplexPeriod() report error when is running during near the day of the time change (Daylight Saving Time).
I will look at this.

Examples:

https://github.com/apache/jmeter/pull/654#issuecomment-811214471
Travis log:
https://travis-ci.com/github/apache/jmeter/jobs/494670417

FAILURE   0,2sec, org.apache.jmeter.functions.TestTimeShiftFunction > testNowWithComplexPeriod()
    java.lang.AssertionError: 
    Expected: the date is within 1 Seconds of ven., 09 avr. 2021 02:29:44.486 PM
         but: the date is ven., 09 avr. 2021 01:29:44.000 PM and 3600 Seconds different
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at org.apache.jmeter.functions.TestTimeShiftFunction.testNowWithComplexPeriod(TestTimeShiftFunction.java:116)
		
		
I also saw this error during my test. During change from CET to CEST:

Expected: the date is within 1 Seconds of Tue, 06 Apr 2021 05:31:16.836 PM
     but: the date is Tue, 06 Apr 2021 06:31:16.000 PM and 3599 Seconds different
java.lang.AssertionError: 
Expected: the date is within 1 Seconds of Tue, 06 Apr 2021 05:31:16.836 PM
     but: the date is Tue, 06 Apr 2021 06:31:16.000 PM and 3599 Seconds different
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
at org.apache.jmeter.functions.TestTimeShiftFunction.testNowWithComplexPeriod(TestTimeShiftFunction.java:116)
Comment 1 Mariusz_W 2021-04-03 21:48:50 UTC
The problem may be related to the use of ZonedDateTime in TimeShift.execute() vs LocalDateTime  in test.
Comment 2 Felix Schumacher 2021-04-04 10:42:59 UTC
https://github.com/apache/jmeter/pull/561
Comment 3 Mariusz_W 2021-04-05 12:52:24 UTC
Felix, thanks for the PR, I wasn't aware it existed. 

In current implemetation there is some additional calculations because function (timeShift) always use ZonedDateTime  even for input date without time zone). In such case we have default offset in DataFormatter (in TimeShift.createFormatter there is parseDefaulting(ChronoField.OFFSET_SECONDS, ZonedDateTime.now().getOffset().getTotalSeconds())  ) which may cause errors because now() is used and we get something like 2021-03-27T15:00+02:00 (in CET) which is not existing date. It should be 2021-03-27T15:00+01:00, it's one day before change from CET (+01) to CEST (+02). Additionaly there are some inconsistency - if we use timeShift function as in testNowWithComplexPeriod() where now() is used we can get error as described in issue however  If we use time on input e.g. 2021-03-27T15:00:00  (day before DST) and add "P1D" we don't have error because offset is used in createFormatter and DST is not managed.

From my point of view (and docs) I deduce that this function (timeShift) should by simple one point interface to Java time api. Function should recognize input format with time zone or without time zone and make adequate calculations. When time zone is present in input format the  ZoneDateTime class should be used. In case of time without zone LocalDateTime class should be used. Duration class is used not Period class of course in implementation. This is first approach. 

The second approach is always use ZonedDateTime (even for input string without zone) but make calculations based on system zone. This is like current implementation but in TimeShift.createFormatter  this line is not used .parseDefaulting(ChronoField.OFFSET_SECONDS, ZonedDateTime.now().getOffset().getTotalSeconds())  but withZone(ZoneId.systemDefault())  is used. This change prevents problem I described at beginning. I think personally that this approach is less intuitive because on input there is no time zone but timeShift is using time zone calculations internally. When we use LocalDateTime the operation "P1D" on Duration and Period get the same result on DSP because zone is not used in calculations.

After this lenghty disquisition:) I think that maybe not only test in  test class should  be changed but also TimeShift implementation. 
What do you think? Do I miss something?

I will try to work on it and present some pull request.
Comment 4 Mariusz_W 2021-04-15 09:07:30 UTC
Hi I have some thoughts to discuss. 
My suggestions for changes. What do you think?

"value to shift" - this data should be mandatory.  
Currently, it is not mandatory in the documentation, but it is also not given what the default value is. Due to the task of the function, it seems that it needs to be specified. 
https://jmeter.apache.org/usermanual/functions.html#__timeShift
test to change: TestTimeShiftFunction.testDefault()
And change in docs.

"Format" 
https://jmeter.apache.org/usermanual/functions.html#__timeShift
If given, it should be correct. Throw exception if DateTimeFormatter can  not be created.
test to change: TestTimeShiftFunction.testWrongFormatDate()

"value to shift"
https://jmeter.apache.org/usermanual/functions.html#__timeShift
If given, it should be correct. Throw exception if Duration ( Duration.parse(amountToShift);) can not be created.
test to change: TestTimeShiftFunction.testWrongAmountToAdd()
Comment 5 Felix Schumacher 2021-06-06 13:39:10 UTC
Sorry for the late reaction.

It is probably best to switch between input with and without time zones and use the correct internal timedate implementation. If you have a simple implementation that recognizes if a time zone has been specified, feel free to attach it here or open a PR.
Comment 6 Mariusz_W 2021-06-08 06:28:09 UTC
I am preparing PR to discuss (I will change bug status when it will be ready).