Copilot commented on code in PR #1801:
URL: https://github.com/apache/commons-lang/pull/1801#discussion_r4229637580
##########
src/test/java/org/apache/commons/lang3/time/DateUtilsTest.java:
##########
@@ -337,6 +337,53 @@ void testAddDays() throws Exception {
assertNullPointerException(() -> DateUtils.addDays(null, 0));
}
+ @Test
+ void testAddDaysDaylightSavingTime() {
+ final TimeZone defaultZone = TimeZone.getDefault();
+ try {
+ final TimeZone brusselsTz =
TimeZone.getTimeZone("Europe/Brussels");
+ TimeZone.setDefault(brusselsTz);
Review Comment:
`TimeZone.setDefault(...)` mutates a JVM-global singleton and can create
flaky behavior when tests are executed in parallel (JUnit 5 / Surefire can be
configured for parallelism). Consider guarding this test with a JUnit 5
resource lock (e.g., `@ResourceLock` on system properties / time zone) or
otherwise ensuring it cannot run concurrently with other tests that depend on
the default time zone.
##########
src/main/java/org/apache/commons/lang3/time/DateUtils.java:
##########
@@ -214,6 +218,11 @@ private enum ModifyType {
* Adds to a date returning a new object.
* The original {@link Date} is unchanged.
*
+ * <p>
+ * This method uses a {@link Calendar} internally with the default time
zone and locale,
+ * and may be affected by Daylight Saving Time (DST) transitions.
+ * </p>
Review Comment:
The PR description states that new public overloads were added (e.g.,
`add(Date, int, int, TimeZone)` and `addDays(Date, int, TimeZone)`), but in the
provided `DateUtils.java` diff hunk only Javadoc changes are visible (no new
overload signatures). If the overload implementations are missing from the PR,
they should be added; if they exist elsewhere, consider including them in the
PR summary/diff context shown or adjusting the description to match the actual
changes.
##########
src/test/java/org/apache/commons/lang3/time/DateUtilsTest.java:
##########
@@ -337,6 +337,53 @@ void testAddDays() throws Exception {
assertNullPointerException(() -> DateUtils.addDays(null, 0));
}
+ @Test
+ void testAddDaysDaylightSavingTime() {
+ final TimeZone defaultZone = TimeZone.getDefault();
+ try {
+ final TimeZone brusselsTz =
TimeZone.getTimeZone("Europe/Brussels");
+ TimeZone.setDefault(brusselsTz);
+
+ // 2012-03-25 00:00:00 CET: Brussels spring DST transition
(23-hour day)
+ final Calendar calSpring = Calendar.getInstance(brusselsTz);
+ calSpring.clear();
+ calSpring.set(2012, Calendar.MARCH, 25, 0, 0, 0);
+ final Date inputSpring = calSpring.getTime();
+ assertEquals(1332630000000L, inputSpring.getTime());
Review Comment:
These hard-coded epoch-millisecond assertions make the test more brittle
than necessary (they depend on specific historical TZ rules/offsets from the
runtime tzdb). Since the test already asserts the resulting calendar fields and
the 23h/25h deltas, consider removing these fixed-epoch checks or deriving
expectations from `Calendar`/offsets so the intent (DST boundary behavior)
remains stable even if tzdb rules change.
##########
src/test/java/org/apache/commons/lang3/time/DateUtilsTest.java:
##########
@@ -337,6 +337,53 @@ void testAddDays() throws Exception {
assertNullPointerException(() -> DateUtils.addDays(null, 0));
}
+ @Test
+ void testAddDaysDaylightSavingTime() {
+ final TimeZone defaultZone = TimeZone.getDefault();
+ try {
+ final TimeZone brusselsTz =
TimeZone.getTimeZone("Europe/Brussels");
+ TimeZone.setDefault(brusselsTz);
+
+ // 2012-03-25 00:00:00 CET: Brussels spring DST transition
(23-hour day)
+ final Calendar calSpring = Calendar.getInstance(brusselsTz);
+ calSpring.clear();
+ calSpring.set(2012, Calendar.MARCH, 25, 0, 0, 0);
+ final Date inputSpring = calSpring.getTime();
+ assertEquals(1332630000000L, inputSpring.getTime());
+
+ // In Europe/Brussels: calendar day advances to 2012-03-26
00:00:00 CEST (23 hours elapsed)
+ final Date outputSpring = DateUtils.addDays(inputSpring, 1);
+ assertEquals(1332712800000L, outputSpring.getTime());
Review Comment:
These hard-coded epoch-millisecond assertions make the test more brittle
than necessary (they depend on specific historical TZ rules/offsets from the
runtime tzdb). Since the test already asserts the resulting calendar fields and
the 23h/25h deltas, consider removing these fixed-epoch checks or deriving
expectations from `Calendar`/offsets so the intent (DST boundary behavior)
remains stable even if tzdb rules change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
