You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@jmeter.apache.org by bu...@apache.org on 2021/04/03 21:41:20 UTC
[Bug 65217] New: Problem in test for timeShift function
https://bz.apache.org/bugzilla/show_bug.cgi?id=65217
Bug ID: 65217
Summary: Problem in test for timeShift function
Product: JMeter
Version: 5.4.1
Hardware: PC
Status: NEW
Severity: minor
Priority: P2
Component: Main
Assignee: issues@jmeter.apache.org
Reporter: mawasak@gmail.com
Target Milestone: JMETER_5.5
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)
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 65217] Problem in test for timeShift function
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65217
--- Comment #7 from Felix Schumacher <fe...@internetallee.de> ---
Hi Mariuz, any progress here?
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 65217] Problem in test for timeShift function
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65217
Felix Schumacher <fe...@internetallee.de> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |NEEDINFO
--- Comment #5 from Felix Schumacher <fe...@internetallee.de> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 65217] Problem in test for timeShift function
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65217
--- Comment #4 from Mariusz_W <ma...@gmail.com> ---
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()
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 65217] Problem in test for timeShift function
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65217
--- Comment #2 from Felix Schumacher <fe...@internetallee.de> ---
https://github.com/apache/jmeter/pull/561
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 65217] Problem in test for timeShift function
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65217
--- Comment #3 from Mariusz_W <ma...@gmail.com> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 65217] Problem in test for timeShift function
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65217
Mariusz_W <ma...@gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
OS| |All
--- Comment #1 from Mariusz_W <ma...@gmail.com> ---
The problem may be related to the use of ZonedDateTime in TimeShift.execute()
vs LocalDateTime in test.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 65217] Problem in test for timeShift function
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65217
Mariusz_W <ma...@gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEEDINFO |NEW
--- Comment #10 from Mariusz_W <ma...@gmail.com> ---
(In reply to Felix Schumacher from comment #8)
> On the question about the default value of 'value to shift', if should be
> mandatory. I tend to document the current default, that is no shift will be
> done on an empty value.
Some thoughts:
Don't you find this function is too flexible and some cases are too implicite?
Maybe it should be more explicit regarding parameters and errors. Maybe all (or
almost all) parameters should be mandatory (format especially). Exceptions
shouldn't be catched. The function should distinguish whether the pattern is
with or without a zone and take the appropriate class
ZonedDateTime/LocalDateTime (now ZonedDateTime is always used with the system
zone identifier). I wonder if in this case it would not be better to create a
new function, e.g. timeShift2, because then the api (mandatory parameters) will
change. Additionally, there is a matter of distinguishing the timeToShift
parameter - now it is always treated as Duration class. Maybe new additional
function should be created eg. timeShift3 (which will treat input parameter
"value to shift" as Period class)?
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 65217] Problem in test for timeShift function
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65217
--- Comment #8 from Felix Schumacher <fe...@internetallee.de> ---
On the question about the default value of 'value to shift', if should be
mandatory. I tend to document the current default, that is no shift will be
done on an empty value.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 65217] Problem in test for timeShift function
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65217
--- Comment #6 from Mariusz_W <ma...@gmail.com> ---
I am preparing PR to discuss (I will change bug status when it will be ready).
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 65217] Problem in test for timeShift function
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65217
Felix Schumacher <fe...@internetallee.de> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |NEEDINFO
--- Comment #11 from Felix Schumacher <fe...@internetallee.de> ---
The problem with functions already in use is, that we generally don't know how
they are used by our users. Changing the contract of those functions without a
good reason is not good.
Now, we agree, that we have a problem with the current state of the function
and therefore have to do something about it.
*default values* while not the best, they are not problematic per se, and I
think, we can leave them as is.
*timezoned vs. local time* the original implementation was local time, only.
The change to support timezoned data brought the current inconsistency into
play. Changing every format to use timezones, was driven by the fact, that we
could use a simple code path for both formats. Do you now a way to
differentiate formats without trying to parse it as a local time format and
catching an exception on a timezoned one?
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 65217] Problem in test for timeShift function
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65217
--- Comment #9 from Felix Schumacher <fe...@internetallee.de> ---
fschumacher pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/jmeter.git.
from a334994 Document default for shift value on timeShift function
new 3e09665 Skip broken test when DST change is near
new e6b6fa7 Remove public modifiers from JUnit test class and methods
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../jmeter/functions/TestTimeShiftFunction.java | 68 ++++++++++++++--------
1 file changed, 45 insertions(+), 23 deletions(-)
--
You are receiving this mail because:
You are the assignee for the bug.