jqwik-team / jqwik

Property-Based Testing on the JUnit Platform
http://jqwik.net
Eclipse Public License 2.0
571 stars 64 forks source link

Add Generation of DateTimes #175

Closed jlink closed 3 years ago

jlink commented 3 years ago

Testing Problem

Now that generation of dates (https://github.com/jlink/jqwik/issues/140) and generation of times (https://github.com/jlink/jqwik/issues/154) is done, the last step is to add generation of DateTimes.

Suggested Solution

Add the capability to the "jqwik-time" module to generate date-time values.

Foundation for generation are the types in java.time. Mappers to "old" JDK classes like Date, Calendar etc. should be provided for convenience.

Add a new class to the base package net.jqwik.api.time: net.jqwik.time.api.DateTimes

jlink commented 3 years ago

@zinki97 You might want to start with an API suggestion. Target release is 1.5.2.

zinki97 commented 3 years ago

Design idea:

Whenever possible, all configurations should be delegated to embedded arbitraries that combine date and time functions. DateTimes should combine constraints (methods and annotations) of their date and time-based arbitraries.

zinki97 commented 3 years ago

@jlink What do you think about the design idea?

jlink commented 3 years ago

To be consisten with class Dates I suggest the following changes:

I hopen that datesAsDate() and datesAsCalendar() could "just" use dateTimesAsDates() and dateTimesAsCalendar() with precision set to DAYS.

zinki97 commented 3 years ago

To be consisten with class Dates I suggest the following changes:

  • localDateTimes() -> dateTimes()
  • dates() -> dateTimesAsDate()
  • calendars() -> dateTimesAsCalendar()

I hopen that datesAsDate() and datesAsCalendar() could "just" use dateTimesAsDates() and dateTimesAsCalendar() with precision set to DAYS.

You're right, it is better. Then let's start with the implementation.

zinki97 commented 3 years ago

Which idea is the best? @jlink

idea 1) is possible but bad for delegation,

I vote for idea 3).

Edit: idea 1) between(LocalDateTime, LocalDateTime) atTheEarliest(LocalDateTime) atTheLatest(LocalDateTime) dateBetween(LocalDate, LocalDate) timeBetween(LocalTime, LocalTime)

idea 2) dateBetween(LocalDate, LocalDate) timeBetween(LocalTime, LocalTime)

idea 3): between(LocalDate, LocalDate) atTheEarliest(LocalDate) atTheLatest(LocalDate) timeBetween(LocalTime, LocalTime)

jlink commented 3 years ago

My favourite is to start with LocalDateTimeArbitrary.between(LocalDateTime, LocalDateTime), which is necessary to keep up API symmetry. In order to have smaller PRs I suggest to tackle just this single method.

zinki97 commented 3 years ago

In order to have smaller PRs I suggest to tackle just this single method.

What about the rest? I think timeBetween is also very important. And what about methods like hourBetween or yearBetween?

I'll show you my first design from yesterday (I've already implemented some of the test cases in my local version):

To add: date/time between methods yearBetween(Year min, Year max) yearBetween(int min, int max) monthBetween(Month min, Month max) monthBetween(int min, int max) onlyMonths(Month... months) dayOfMonthBetween(int min, int max) onlyDaysOfWeek(DayOfWeek... daysOfWeek) leapYears(boolean withLeapYears) hourBetween(int min, int max) minuteBetween(int min, int max) secondBetween(int min, int max) ofPrecision(ChronoUnit ofPrecision)

Most of these methods can be delegated to other arbitraries.

jlink commented 3 years ago

In order to have smaller PRs I suggest to tackle just this single method.

What about the rest? I think timeBetween is also very important. And what about methods like hourBetween or yearBetween?

Let‘s use that as a learning experience for incremental design. Just implement between() and try not to think of all the other stuff that may or may not come afterwards.

zinki97 commented 3 years ago

Yes, but that was what I meant in my design idea, so it was in my incremental design I guess:

DateTimes should combine constraints (methods and annotations) of their date and time-based arbitraries.

zinki97 commented 3 years ago

Yes, but that was what I meant in my design idea, so it was in my incremental design I guess:

DateTimes should combine constraints (methods and annotations) of their date and time-based arbitraries.

But you're right, maybe it was not clear enough ;)

zinki97 commented 3 years ago

In the next step I'll implement methods to constrain the date

zinki97 commented 3 years ago

In the next step I'll implement the remaining methods to constrain the date

jlink commented 3 years ago

Missing after last PR has been merged:

@zinki97 What else? I'd also consider to change existing leapYears methods to withLeapYears. What do you think?

zinki97 commented 3 years ago

Missing after last PR has been merged:

  • Instant
  • OffsetDateTime
  • Calendar and Date
  • leapYears

@zinki97 What else? I'd also consider to change existing leapYears methods to withLeapYears. What do you think?

LocalDateTimeArbitrary:

Other classes:

zinki97 commented 3 years ago

I'd also consider to change existing leapYears methods to withLeapYears. What do you think?

I've overthinked that one night long. withLeapYears() is not practicable. withLeapYears(boolean withLeapYears) doesn't sound good. One solution is withoutLeapYears(). Another solution is an other name for boolean withLeapYears. What do you think @jlink ? Do you have an idea for a name?

jlink commented 3 years ago

@zinki97 What feels wrong with withLeapYears(boolean withLeapYears)? Just the variable name? Maybe withoutLeapYears() communicates best that the default is with leap years. Or maybe get rid of it altogether. Whoever wants to exclude leap years can easily add a filter since a function Year.isLeap() already exists.

jlink commented 3 years ago

Or maybe get rid of it altogether. Whoever wants to exclude leap years can easily add a filter since a function Year.isLeap() already exists.

Yes, I think that's my current stance on the issue.

zinki97 commented 3 years ago

Okay so I should remove all withLeapYears annotations and methods in all classes?

zinki97 commented 3 years ago

Or maybe get rid of it altogether. Whoever wants to exclude leap years can easily add a filter since a function Year.isLeap() already exists.

Yes, I think that's my current stance on the issue.

I've overthinked that. I've used the leapYears in some jqwik tests to generate years without leap years and found it very useful.

zinki97 commented 3 years ago

Here is a sample test. I've used the method/annotation to prevent exceptions from dates that doesn't exist.

@Property
void dateBetweenYearMustNotBelow1(
    @ForAll @IntRange(min = Year.MIN_VALUE, max = 0) int year,
    @ForAll @LeapYears(withLeapYears = false) LocalDate date
) {
    LocalDate effective = date.withYear(year);
    assertThatThrownBy(
        () -> DateTimes.dateTimes().dateBetween(effective, effective)
    ).isInstanceOf(IllegalArgumentException.class);
}
zinki97 commented 3 years ago

Here is a sample test. I've used the method/annotation to prevent exceptions from dates that doesn't exist.

Okay, you can use yearBetween here as well.

jlink commented 3 years ago

The question for me is: In which domain would be filtering out leap years be useful? Is that a problem which is frequent enough to warrant quite a bit of code and maintaining it in a generic library, which I consider jqwik to be.

zinki97 commented 3 years ago

In the next step I'll implement methods to constrain the time

zinki97 commented 3 years ago

For those who don't know it: Working on jqwik is part of my final exam at the university. Now it's time for my written exam.

After completing my written exam I'll implement the following classes:

zinki97 commented 3 years ago

Bug list:

zinki97 commented 3 years ago

A few weeks ago you said that the tests in Calender could look like this: Calender behaves likes LocalDateTime. Can you please give an example how such a test case could look like? @jlink

jlink commented 3 years ago

Looking over Calendar I guess I was wrong. Calendar instances have a timezone. Moreover, calendar instances can be created from different calendar types. So it might make sens to generate instances of the concrete subclasses Gregorian, Buddhist and JapaneseImperial. Maybe Buddhist and japanese imperial are out of scope.

zinki97 commented 3 years ago

Next step is OffsetDateTimeArbitrary - but do we also need ClockArbitrary? ( https://docs.oracle.com/javase/8/docs/api/java/time/Clock.html )

jlink commented 3 years ago

I currently can't think of a good use case for generating clocks. Let's wait till something concrete comes up.

zinki97 commented 3 years ago

I currently can't think of a good use case for generating clocks.

same

jlink commented 3 years ago

Not implemented yet: