Copilot commented on code in PR #2653: URL: https://github.com/apache/groovy/pull/2653#discussion_r3510877746
########## subprojects/groovy-dateutil/src/main/java/org/apache/groovy/dateutil/BaseDuration.java: ########## @@ -0,0 +1,187 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.groovy.dateutil; + +import java.time.temporal.ChronoUnit; +import java.util.ArrayList; +import java.util.Calendar; +import java.util.Date; +import java.util.List; + +/** + * Base class for date and time durations (dequirked, {@code java.util.Date}-flavored). + * <p> + * This is the B1 prototype: the same value-class model as the legacy + * {@code groovy.time.BaseDuration}, but with the historical quirks removed: + * <ul> + * <li><b>ago / from.now no longer zero the time-of-day</b> for date-based durations — + * the behavior is now uniform across all durations (time is preserved), so a single + * implementation lives here rather than divergent overrides per subclass.</li> + * <li><b>{@link #toMilliseconds()} is deterministic</b> — datum-dependent amounts (years, + * months) use {@link ChronoUnit}'s estimated durations rather than resolving against + * {@code new Date()} ("now"), so the result no longer depends on when it is called.</li> + * </ul> + * + * @see Duration + */ +public abstract class BaseDuration implements Comparable<BaseDuration> { + /** Estimated milliseconds in a year (ChronoUnit.YEARS = 365.2425 days). */ + static final long MILLIS_PER_YEAR = ChronoUnit.YEARS.getDuration().toMillis(); + /** Estimated milliseconds in a month (ChronoUnit.MONTHS = 30.436875 days). */ + static final long MILLIS_PER_MONTH = ChronoUnit.MONTHS.getDuration().toMillis(); + + protected final int years; + protected final int months; + protected final int days; + protected final int hours; + protected final int minutes; + protected final int seconds; + protected final int millis; + + protected BaseDuration(final int years, final int months, final int days, final int hours, final int minutes, final int seconds, final int millis) { + this.years = years; + this.months = months; + this.days = days; + this.hours = hours; + this.minutes = minutes; + this.seconds = seconds; + this.millis = millis; + } + + protected BaseDuration(final int days, final int hours, final int minutes, final int seconds, final int millis) { + this(0, 0, days, hours, minutes, seconds, millis); + } + + public int getYears() { return this.years; } + public int getMonths() { return this.months; } + public int getDays() { return this.days; } + public int getHours() { return this.hours; } + public int getMinutes() { return this.minutes; } + public int getSeconds() { return this.seconds; } + public int getMillis() { return this.millis; } + + /** + * Adds this duration to the supplied date. + */ + public Date plus(final Date date) { + final Calendar cal = Calendar.getInstance(); + cal.setTime(date); + cal.add(Calendar.YEAR, this.years); + cal.add(Calendar.MONTH, this.months); + cal.add(Calendar.DAY_OF_YEAR, this.days); + cal.add(Calendar.HOUR_OF_DAY, this.hours); + cal.add(Calendar.MINUTE, this.minutes); + cal.add(Calendar.SECOND, this.seconds); + cal.add(Calendar.MILLISECOND, this.millis); + return cal.getTime(); + } + + /** + * Converts this duration to milliseconds. + * <p> + * DEQUIRKED (B): deterministic. Years and months use {@link ChronoUnit} estimates + * (a year is exactly 12 estimated months, so {@code 1.year == 12.months}); days and + * below are exact assuming 24-hour days. The result never depends on the current date. + */ + public long toMilliseconds() { + return years * MILLIS_PER_YEAR + + months * MILLIS_PER_MONTH + + days * 86_400_000L + + hours * 3_600_000L + + minutes * 60_000L + + seconds * 1_000L + + millis; + } + + /** + * The date this duration ago. + * <p> + * DEQUIRKED (A): the time-of-day is preserved (no flooring to midnight), + * uniformly for all duration kinds. + */ + public Date getAgo() { + final Calendar cal = Calendar.getInstance(); + cal.add(Calendar.YEAR, -this.years); + cal.add(Calendar.MONTH, -this.months); + cal.add(Calendar.DAY_OF_YEAR, -this.days); + cal.add(Calendar.HOUR_OF_DAY, -this.hours); + cal.add(Calendar.MINUTE, -this.minutes); + cal.add(Calendar.SECOND, -this.seconds); + cal.add(Calendar.MILLISECOND, -this.millis); + return cal.getTime(); + } + + /** + * Helper for computing dates relative to the current time. + * DEQUIRKED (A): time-of-day preserved. + */ + public From getFrom() { + return new From() { + @Override + public Date getNow() { + final Calendar cal = Calendar.getInstance(); + cal.add(Calendar.YEAR, BaseDuration.this.years); + cal.add(Calendar.MONTH, BaseDuration.this.months); + cal.add(Calendar.DAY_OF_YEAR, BaseDuration.this.days); + cal.add(Calendar.HOUR_OF_DAY, BaseDuration.this.hours); + cal.add(Calendar.MINUTE, BaseDuration.this.minutes); + cal.add(Calendar.SECOND, BaseDuration.this.seconds); + cal.add(Calendar.MILLISECOND, BaseDuration.this.millis); + return cal.getTime(); + } + }; + } + + @Override + public int compareTo(BaseDuration other) { + return Long.signum(toMilliseconds() - other.toMilliseconds()); + } Review Comment: `compareTo` uses subtraction (`toMilliseconds() - other.toMilliseconds()`), which can overflow and return the wrong sign for large values, breaking the `Comparable` contract (inconsistent ordering in sorted collections). Prefer `Long.compare` to avoid overflow in the comparison step. ########## subprojects/groovy-dateutil/src/test/groovy/org/apache/groovy/dateutil/TimeCategoryTest.groovy: ########## @@ -0,0 +1,282 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.groovy.dateutil + +import org.junit.jupiter.api.Test + +import static java.util.Calendar.DAY_OF_YEAR +import static java.util.Calendar.MONTH +import static org.junit.jupiter.api.Assertions.assertTrue + +/** + * Tests the {@link org.apache.groovy.dateutil.TimeCategory} class, the dequirked + * {@code java.util.Date}-flavored parallel to the legacy {@code groovy.time.TimeCategory}. + * The arithmetic assertions are ported verbatim from the legacy suite; the final section + * exercises the two behaviors that were intentionally changed ("dequirked"). + */ +class TimeCategoryTest { + + @Test + void testDurationArithmeticOnMilliseconds() { + use(TimeCategory) { + def midnight = new Date(100, 0, 1, 0, 0, 0) + def oneSecondPastMidnight = new Date(100, 0, 1, 0, 0, 1) + def twoSecondsPastMidnight = new Date(100, 0, 1, 0, 0, 2) + + assert (midnight + 1000.millisecond) == oneSecondPastMidnight + assert (midnight + 2000.milliseconds) == twoSecondsPastMidnight + assert (twoSecondsPastMidnight - 1000.millisecond) == oneSecondPastMidnight + assert (twoSecondsPastMidnight - 2000.milliseconds) == midnight + } + } + + @Test + void testDurationArithmeticOnSeconds() { + use(TimeCategory) { + def midnight = new Date(100, 0, 1, 0, 0, 0) + def oneSecondPastMidnight = new Date(100, 0, 1, 0, 0, 1) + def twoSecondsPastMidnight = new Date(100, 0, 1, 0, 0, 2) + + assert (midnight + 1.second) == oneSecondPastMidnight + assert (midnight + 2.seconds) == twoSecondsPastMidnight + assert (twoSecondsPastMidnight - 1.second) == oneSecondPastMidnight + assert (twoSecondsPastMidnight - 2.seconds) == midnight + } + } + + @Test + void testDurationArithmeticOnMinutes() { + use(TimeCategory) { + def midnight = new Date(100, 0, 1, 0, 0, 0) + def oneMinutePastMidnight = new Date(100, 0, 1, 0, 1, 0) + def twoMinutesPastMidnight = new Date(100, 0, 1, 0, 2, 0) + + assert (midnight + 1.minute) == oneMinutePastMidnight + assert (midnight + 2.minutes) == twoMinutesPastMidnight + assert (twoMinutesPastMidnight - 60.seconds) == oneMinutePastMidnight + assert (twoMinutesPastMidnight - 1.minute) == oneMinutePastMidnight + assert (twoMinutesPastMidnight - 120.seconds) == midnight + assert (twoMinutesPastMidnight - 2.minutes) == midnight + } + } + + @Test + void testDurationArithmeticOnHours() { + use(TimeCategory) { + def midnight = new Date(100, 0, 1, 0, 0, 0) + def oneAM = new Date(100, 0, 1, 1, 0, 0) + def twoAM = new Date(100, 0, 1, 2, 0, 0) + + assert (midnight + 1.hour) == oneAM + assert (midnight + 2.hours) == twoAM + assert (twoAM - 3600.seconds) == oneAM + assert (twoAM - 1.hour) == oneAM + assert (twoAM - 7200.seconds) == midnight + assert (twoAM - 2.hours) == midnight + } + } + + @Test + void testDurationArithmeticOnDays() { + use(TimeCategory) { + def januaryFirst = new Date(100, 0, 1, 0, 0, 0) + def januarySecond = new Date(100, 0, 2, 0, 0, 0) + def januaryThird = new Date(100, 0, 3, 0, 0, 0) + + assert (januaryFirst + 1.day) == januarySecond + assert (januaryFirst + 2.days) == januaryThird + assert (januaryThird - 1.day) == januarySecond + assert (januaryThird - 2.days) == januaryFirst + } + } + + @Test + void testDurationArithmeticOnWeeks() { + use(TimeCategory) { + def firstWeek = new Date(100, 0, 1, 0, 0, 0) + def secondWeek = new Date(100, 0, 8, 0, 0, 0) + def thirdWeek = new Date(100, 0, 15, 0, 0, 0) + + assert (firstWeek + 1.week) == secondWeek + assert (firstWeek + 2.weeks) == thirdWeek + assert (thirdWeek - 1.week) == secondWeek + assert (thirdWeek - 2.weeks) == firstWeek + } + } + + @Test + void testDurationArithmeticOnMonths() { + use(TimeCategory) { + def january = new Date(100, 0, 1, 0, 0, 0) + def february = new Date(100, 1, 1, 0, 0, 0) + def march = new Date(100, 2, 1, 0, 0, 0) + + assert (january + 1.month) == february + assert (january + 2.months) == march + assert (march - 1.month) == february + assert (march - 2.months) == january + } + } + + @Test + void testDurationArithmeticOnYears() { + use(TimeCategory) { + def firstYear = new Date(100, 0, 1, 0, 0, 0) + def secondYear = new Date(101, 0, 1, 0, 0, 0) + def thirdYear = new Date(102, 0, 1, 0, 0, 0) + + assert (firstYear + 1.year) == secondYear + assert (firstYear + 2.years) == thirdYear + assert (thirdYear - 1.year) == secondYear + assert (thirdYear - 2.years) == firstYear + } + } + + @Test + void testDateSubtractionOnSeconds() { + use(TimeCategory) { + def current = new Date(100, 0, 1, 0, 0, 0) + def oneSecondLater = new Date(100, 0, 1, 0, 0, 1) + def twoSecondsLater = new Date(100, 0, 1, 0, 0, 2) + + assert (oneSecondLater - current).seconds == 1 + assert (twoSecondsLater - current).seconds == 2 + } + } + + @Test + void testDateSubtractionOnMinutes() { + use(TimeCategory) { + def current = new Date(100, 0, 1, 0, 0, 0) + assert (new Date(100, 0, 1, 0, 1, 0) - current).minutes == 1 + assert (new Date(100, 0, 1, 0, 2, 0) - current).minutes == 2 + } + } + + @Test + void testDateSubtractionOnHours() { + use(TimeCategory) { + def current = new Date(100, 0, 1, 0, 0, 0) + assert (new Date(100, 0, 1, 1, 0, 0) - current).hours == 1 + assert (new Date(100, 0, 1, 2, 0, 0) - current).hours == 2 + } + } + + @Test + void testDateSubtractionOnDays() { + use(TimeCategory) { + def current = new Date(100, 0, 1, 0, 0, 0) + assert (new Date(100, 0, 2, 0, 0, 0) - current).days == 1 + assert (new Date(100, 0, 3, 0, 0, 0) - current).days == 2 + } + } + + @Test + void testDateSubtraction_NoYearsOrMonths() { + use(TimeCategory) { + def result = new Date(102, 0, 1, 0, 0, 0) - new Date(100, 0, 1, 0, 0, 0) + // date subtraction does not populate months and years + assert result.years == 0 + assert result.months == 0 + } + } + + @Test + void testToStringForNegativeValues() { + use(TimeCategory) { + def t1 = Calendar.instance.time + def t2 = t1 - 4.seconds + 2.milliseconds + def t3 = t1 + 4.seconds + 2.milliseconds + def t4 = t1 - 4.seconds - 2.milliseconds + def t5 = t1 + 4.seconds - 2.milliseconds + def t6 = t1 - 2.milliseconds + def t7 = t1 + 2.milliseconds + assert (t1 - t2).toString() == '3.998 seconds' + assert (t1 - t3).toString() == '-4.002 seconds' + assert (t1 - t4).toString() == '4.002 seconds' + assert (t1 - t5).toString() == '-3.998 seconds' + assert (t1 - t6).toString() == '0.002 seconds' + assert (t1 - t7).toString() == '-0.002 seconds' + } + } + + @Test + void testToStringForOverflow() { + use(TimeCategory) { + def t = 800.milliseconds + 300.milliseconds + assert t.toString() == '1.100 seconds' + } + } + + @Test + void testZeroDurationFromNowIsNow() { + use(TimeCategory) { + // Replaces the legacy testDateEquality, whose reproducibility relied on from.now + // flooring to midnight. Dequirked, a zero-length duration from now is (approximately) + // now, with the time-of-day preserved. + def reference = System.currentTimeMillis() + Date result = 0.days.from.now + assert Math.abs(result.time - reference) < 2000 + } Review Comment: This assertion can be flaky under heavy load because it assumes the `.from.now` call happens within 2 seconds of capturing `reference`. Measuring `before`/`after` around the call makes the test robust regardless of scheduling delays. ########## subprojects/groovy-dateutil/src/test/groovy/org/apache/groovy/dateutil/DurationTest.groovy: ########## @@ -0,0 +1,171 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.groovy.dateutil + +import org.junit.jupiter.api.Test + +import java.time.temporal.ChronoUnit + +import static java.util.Calendar.DAY_OF_YEAR +import static java.util.Calendar.MONTH +import static java.util.Calendar.WEEK_OF_YEAR +import static org.junit.jupiter.api.Assertions.assertEquals +import static org.junit.jupiter.api.Assertions.assertNotNull +import static org.junit.jupiter.api.Assertions.assertTrue + +/** + * Tests for the dequirked {@code org.apache.groovy.dateutil} Duration hierarchy. + */ +class DurationTest { + + @Test + void testFixedDurationArithmetic() { + use(TimeCategory) { + def oneDay = 2.days - 1.day + assert oneDay.toMilliseconds() == (24 * 60 * 60 * 1000) + + oneDay = 2.days - 1.day + 24.hours - 1440.minutes + assert oneDay.toMilliseconds() == (24 * 60 * 60 * 1000) + } + } + + @Test + void testDurationToString() { + use(TimeCategory) { + assert (0.years).toString() == "0" + assert (3.years + 8.months + 4.days + 2.hours + 5.minutes + 12.milliseconds).toString() == + "3 years, 8 months, 4 days, 2 hours, 5 minutes, 0.012 seconds" + assert (4.days + 2.hours + 5.minutes + 12.milliseconds).toString() == "4 days, 2 hours, 5 minutes, 0.012 seconds" + assert (4.days + 2.hours + 5.minutes).toString() == "4 days, 2 hours, 5 minutes" + assert (5.seconds).toString() == "5.000 seconds" + assert ((-12).milliseconds).toString() == "-0.012 seconds" + assert (5.seconds + 12.milliseconds).toString() == "5.012 seconds" + assert (5.seconds + 1012.milliseconds).toString() == "6.012 seconds" + } + } + + @Test + void testDurationComparison() { + use(TimeCategory) { + assert 1.week < 2.weeks + assert 3.weeks <= 4.weeks + assert 10.seconds == 10.seconds + assert 4.months >= 1.month + assert 10.days > 2.days + assert 3.months > 2.months + assert 1.second < 1.year + assert 1.day > 1000.seconds + assert 10.weeks < 10.years + } + } + + @Test + void testConstructor() { + def duration = new Duration(1, 2, 3, 4, 5) + assertEquals(1, duration.days) + assertEquals(2, duration.hours) + assertEquals(3, duration.minutes) + assertEquals(4, duration.seconds) + assertEquals(5, duration.millis) + } + + @Test + void testToMilliseconds() { + assertEquals(24 * 60 * 60 * 1000L, new Duration(1, 0, 0, 0, 0).toMilliseconds()) + def d = new Duration(1, 2, 3, 4, 5) + assertEquals(((((1L * 24 + 2) * 60 + 3) * 60 + 4) * 1000) + 5, d.toMilliseconds()) + } + + @Test + void testDatumDependentToMillisecondsIsDeterministic() { // DEQUIRK B + // years/months use fixed ChronoUnit estimates; no dependence on the current date + assertEquals(ChronoUnit.YEARS.duration.toMillis(), new DatumDependentDuration(1, 0, 0, 0, 0, 0, 0).toMilliseconds()) + assertEquals(ChronoUnit.MONTHS.duration.toMillis(), new DatumDependentDuration(0, 1, 0, 0, 0, 0, 0).toMilliseconds()) + // a year is exactly twelve estimated months + assertEquals(new DatumDependentDuration(1, 0, 0, 0, 0, 0, 0).toMilliseconds(), + new DatumDependentDuration(0, 12, 0, 0, 0, 0, 0).toMilliseconds()) + } + + @Test + void testPlusDuration() { + def result = new Duration(1, 2, 3, 4, 5).plus(new Duration(1, 1, 1, 1, 1)) + assertEquals(2, result.days); assertEquals(3, result.hours); assertEquals(4, result.minutes) + assertEquals(5, result.seconds); assertEquals(6, result.millis) + } + + @Test + void testPlusDatumDependentDuration() { + def result = new Duration(1, 0, 0, 0, 0).plus(new DatumDependentDuration(1, 2, 3, 4, 5, 6, 7)) + assertNotNull(result) + assertEquals(1, result.years); assertEquals(2, result.months) + } + + @Test + void testMinusDatumDependentDuration() { + def result = new Duration(10, 5, 30, 20, 100).minus(new DatumDependentDuration(1, 2, 3, 1, 10, 5, 50)) + assertEquals(-1, result.years); assertEquals(-2, result.months); assertEquals(7, result.days) + } + + @Test + void testGetAgoPreservesTimeOfDay() { // DEQUIRK A + def ago = new Duration(1, 0, 0, 0, 0).getAgo() + def expected = Calendar.instance + expected.add(DAY_OF_YEAR, -1) + assertTrue Math.abs(expected.timeInMillis - ago.time) < 2000 + } + + @Test + void testGetFromPreservesTimeOfDay() { // DEQUIRK A + def now = new Duration(3, 0, 0, 0, 0).from.now + def expected = Calendar.instance + expected.add(DAY_OF_YEAR, 3) + assertTrue Math.abs(expected.timeInMillis - now.time) < 2000 + } + + @Test + void testMinutesAgo() { // See GROOVY-3687 - was already un-quirky for time durations + use(TimeCategory) { + def now = Calendar.instance + def before = 10.minutes.ago + now.add(Calendar.MINUTE, -11) + assertTrue now.timeInMillis < before.time + } + } + + @Test + void testDatumDependantArithmetic() { Review Comment: Method name uses the misspelling "Dependant" (vs "Dependent" as in `DatumDependentDuration`). Renaming keeps terminology consistent across the codebase and avoids propagating the typo into test reports. -- 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]
