Assertj-core: ZonedDateTime assertions are not consistent and don't expose isBetween

[puddlejumpr]

Summary

My test sometimes fails when the asserted value matches the (supposed to be inclusive) upper boundary. The example code fails with the exception:

java.lang.AssertionError:
Expecting:
<2019-03-04T10:22:45.622Z>
to be between:
[2019-03-04T10:22:45.622Z[UTC], 2019-03-04T10:22:45.622Z[UTC]]

Example

ZonedDateTime boundary = ZonedDateTime.now();
assertThat(boundary).isBetween(boundary, boundary);    // passes

ZonedDateTime actual = boundary.withFixedOffsetZone();
assertThat(actual.isEqual(boundary)).isTrue();            // passes
assertThat(actual).isBetween(boundary, boundary);         //fails

[joel-costigliola]

Thanks for reporting this !

[joel-costigliola]

@puddlejumpr the reason for the behavior is that actual and boundary have different zones which make them different in regard to their equals and compareTo methods.
Note that as stated in ChronoZonedDateTime.isEqualTo :

This method differs from the comparison in compareTo(java.time.chrono.ChronoZonedDateTime) and equals(java.lang.Object) in that it only compares the instant of the date-time.

which explains why assertThat(actual.isEqual(boundary)).isTrue(); passes.

You will find that assertThat(actual).isEqualTo(boundary); passes too but only because it compares both ZonedDateTime in actual's zone (see this javadoc).

[puddlejumpr]

@joel-costigliola You are right. The documentation of compareTo describes clearly how it is implemented:

The comparison is based first on the instant, then on the local date-time, then on the zone ID, then on the
chronology.

This is why isBetween fails only when the actual value equals to either boundary. I suppose there are combinations when the assertion fails because the actual refers to the same instant as the lower boundary.

Assuming that one would expect isBetween to be concerned only with the instants that the ZonedDateTime objects represent, a custom comparator could be implemented with the help of isEqual (e.g. returning 0 when isEqual is true and use compareTo otherwise).

[joel-costigliola]

Assuming that one would expect isBetween to be concerned only with the instants that the ZonedDateTime objects represent, a custom comparator could be implemented with the help of isEqual (e.g. returning 0 when isEqual is true and use compareTo otherwise).

IMO we should not expect isBetween not to use equals (isEqualTo is a bit dodgy in that regard), I agree introducing a comparator is the best option here. Since it might be useful for users would you be keen to contribute this comparator (name suggestion: ByInstant) ?

[puddlejumpr]

I am a bit confused about the first part of your comment (double nots).
My idea was to change isBetween assertions in AbstractZonedDateTimeAssert to use a comparator different from the default.
Are you suggesting to do something like this instead?

    assertThat(zonedDateTime.withFixedOffsetZone())
            .usingComparator(Comparator.comparing(ZonedDateTime::toInstant))
            .isBetween(zonedDateTime, zonedDateTime);

I have just checked this and it fails with the same error. The code builds but the usingComparator() call does not have any effects since it is inherited from AbstractAssert which changes a different attribute.

[joel-costigliola]

Clarification: isBetween should use equals/compareTo semantic, isEqualTo does not which can be confusing if users actually want to check the zone ids.

You actually found a bug (good catch :wink:) where the comparables instance in AbstractTemporalAssert is not honoring the given comparator.

Adding these lines fixes the issue:

  @Override
  @CheckReturnValue
  public SELF usingComparator(Comparator<? super TEMPORAL> customComparator) {
    return usingComparator(customComparator, null);
  }

  @Override
  @CheckReturnValue
  public SELF usingComparator(Comparator<? super TEMPORAL> customComparator, String customComparatorDescription) {
    this.comparables = new Comparables(new ComparatorBasedComparisonStrategy(customComparator, customComparatorDescription));
    return super.usingComparator(customComparator, customComparatorDescription);
  }

  @Override
  @CheckReturnValue
  public SELF usingDefaultComparator() {
    this.comparables = new Comparables();
    return super.usingDefaultComparator();
  }

I can finish the implementation or I can let you go on with it, it would require though to test the added methods as in AbstractComparableAssert_usingComparator_Test.

Let me know what you prefer.

As a side note, we can't simply make AbstractTemporalAssert to extends AbstractComparableAssert as Temporal is not Comparable.

[puddlejumpr]

I hope I haven't messed up anything with this duplicated reference to the issue.

[joel-costigliola]

@puddlejumpr no, don't worry ;-)

[joel-costigliola]

After looking at your PR (which is good btw), I realized that ZonedDateTime isAfter, isBefore and isEqual ignore the zone id, on the other hand equals and compareTo check it.

I was saying that isBetween should honor ZonedDateTime.equals but I now think I was wrong and that it is better to be consistent with ZonedDateTime.isBefore and ZonedDateTime.isAfter, moreover as isEqualTo already ignores the zone id, that will make all comparison assertions being on the same page (it could simply be implemented by calling ZonedDateTime.isEquals BTW).

What does that mean ? Implementing isBetween with a Comparables based on ZonedDateTime natural comparator does not achieve what we want since it uses ZonedDateTime.compareTo semantics that checks the zone id.

@puddlejumpr sorry about that late realization, it means the work you have done is not exactly what we want (but it was useful anyway since I did not realize the assertions were inconsistent).

A possible way out of it is to set a default Comparables with a ZonedDateTime comparator by instant.
That leaves the possibility for users to set their own comparator so they can use ZonedDateTime natural comparator to include zone id in the comparison.

But to honor the comparator set with usinComparator we need to change AbstractZonedDateTimeAssert.isAfter and other similar methods to use the comparables instead of direct call to ZonedDateTime.isAfter. That is quite a bit of work but I believe it is required to get a consistent behavior.

Let me know if you feel you can tackle it it is not too much work

Oh and BTW, AbstractOffsetDateTimeAssert has the same issue ... (and AbstractLocalDateTimeAssert too in regard to the chronology)

[puddlejumpr]

@joel-costigliola It is not a big deal actually, I am glad that I could help, and honestly, this new approach seems more aligned with the anticipation I had when I reported the issue. :)

Supposing I take up this task, what base do you think I should choose for the changes? Should I just continue my PR? Or create a new issue for usingComparator() and open a new PR for that with the existing content (since it might be useful in itself to be able to change the comparator)?

[joel-costigliola]

@puddlejumpr awesome!

It's going to be easier to do the work in one PR (otherwise we will have dependent PR) so let's continue on the existing one to include both comparator support and make the assertions consistent.

[puddlejumpr]

@joel-costigliola I have just pushed some new commits. Could you please check if they aligned with the solution/approach you have in mind? (The javadocs are not updated yet.)

[joel-costigliola]

@puddlejumpr I haven't fully reviewed all the code but so far I'm really happy with I saw. :+1:

I'll let you know when I finish the review.

[puddlejumpr]

@joel-costigliola This new PR contains the content of the previous one rebased on master, and the changes addressing your review comments.

[joel-costigliola]

@puddlejumpr I'm on vacation right now, I'll have a look when I get back, thanks !

[bergerst]

Bump, I'm having the same problem.