check both isGreaterThan isLessThan

[xenoterracide]

Feature summary

I feel that if I asserThat(...).isGreaterThan it should also verify isLessThan at the same time.

note: I went source diving

Example

assertThat(2).isGreaterThan(1);
// should also do the equivalent of

assertThat(1).isLessThan(2);

By doing this we can half the code required to compare. This ensures that the comparable is implemented correctly.

[scordio]

I'm not sure I understand what you have in mind. In your example, the first assertion fails while the second succeeds.

[xenoterracide]

I should I have used integer's, 🤦🏻‍♂️. Updated the original example. I just want one call to check that both things are true instead of only checking greater than, it should check greater than, and less than.

[quaff]

I just want one call to check that both things are true instead of only checking greater than, it should check greater than, and less than.

I guess you are looking for assertThat(value).isStrictlyBetween(startExclusive, endExclusive)?

[xenoterracide]

I guess you are looking for assertThat(value).isStrictlyBetween(startExclusive, endExclusive)?

no, that would require 3 inputs. What I'm saying is code like this should fail. If this compiled it would pass because it's ONLY checking isGreaterThan. It should also (under the hood) check isLessThan and isNotEqualTo. I think the term is reflexity. If s1 is greater than s2, then s2 must be less than s1 and s1 must not be equal to s2.

  @Test
  void gt() {
    Comparable<String> s1 = that -> 1;
    Comparable<String> s2 = that -> 0;
    assertThat(s1).isGreaterThan(s2);
  }

[xenoterracide]

For the sake of a more complete example

package com.xenoterracide.gradle.semver;

import static org.assertj.core.api.Assertions.assertThat;

import java.util.stream.Stream;
import org.apache.maven.artifact.versioning.ComparableVersion;
import org.gradle.util.VersionNumber;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.ArgumentsProvider;
import org.junit.jupiter.params.provider.ArgumentsSource;
import org.semver4j.Semver;

// not really our test, just test our assumptions about upstream parsing
@Disabled
public class VersionSortTest {

  @ParameterizedTest
  @ArgumentsSource(RcVersionList.class)
  void maven(String thisVersion, String thatVersion) {
    assertThat(new ComparableVersion(thisVersion)).isGreaterThan(new ComparableVersion(thatVersion));
  }

  @Disabled
  @ParameterizedTest
  @ArgumentsSource(RcVersionList.class)
  void gradle(String thisVersion, String thatVersion) {
    assertThat(VersionNumber.parse(thisVersion)).isGreaterThan(VersionNumber.parse(thatVersion));
  }

  @ParameterizedTest
  @ArgumentsSource(RcVersionList.class)
  void semver(String thisVersion, String thatVersion) {
    assertThat(new Semver(thisVersion)).isGreaterThan(new Semver(thatVersion));
  }

  static class RcVersionList implements ArgumentsProvider {

    @Override
    public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
      return Stream.of(
        Arguments.of("0.1.1-alpha.10.17129409589+6.g3aae11c", "0.1.1-alpha.10+1.g3aae11d"),
        Arguments.of("0.1.1-alpha.10.17129409589+6.g3aae11c", "0.1.1-alpha.2.1712940957+2.g3aae11c"),
        Arguments.of("0.1.1-alpha.10.17129409589+6.g3aae11c", "0.1.1-alpha.6.1712940957+6.g3aae11c"),
        Arguments.of("0.1.1-alpha.10.17129409589+6.g3aae11c", "0.1.1-alpha.6.17129409589"),
        Arguments.of("0.1.1-alpha.0.17129409589+6.g3aae11c", "0.1.1-alpha.0.10+1.g3aae11d"),
        Arguments.of("0.1.1-alpha.0.17129409589+6.g3aae11c", "0.1.1-alpha.0.17129409578+2.g3aae11c"),
        Arguments.of("0.1.1-alpha.0.17129409589+6.g3aae11c", "0.1.1-alpha.0.17129409578+10.g3aae11c"),
        Arguments.of("0.1.1-alpha.0.17129409589+6.g3aae11c", "0.1.1-alpha.0.17129409588"),
        Arguments.of("0.1.1-rc.10.17129409589+6.g3aae11b", "0.1.1-alpha.0.10+1.g3aae11d"),
        Arguments.of("0.1.1-rc.10.17129409589+6.g3aae11b", "0.1.1-alpha.0.17129409578+2.g3aae11c"),
        Arguments.of("0.1.1-rc.10.17129409589+6.g3aae11b", "0.1.1-alpha.0.17129409578+10.g3aae11c"),
        Arguments.of("0.1.1-rc.10.17129409589+6.g3aae11b", "0.1.1-alpha.0.17129409588"),
        Arguments.of("0.1.1-rc.10.17129409589+6.g3aae11b", "0.1.1-rc.1.17129409588+6.g3aae11b"),
        Arguments.of("0.1.1-rc.10.17129409589+6.g3aae11b", "0.1.1-rc.1.17129409588"),
        Arguments.of("0.1.1-rc.10", "0.1.1-rc.9")
      );
    }
  }
}

I don't want to isGreaterThan isLessThan and 2x isNotEqualTo for the reflective test. To be fair I control none of these classes so I assume their comparable implementation is generally not massively broken (although it turns out VersionNumber doesn't compare correctly, arguably correctly, but that's a different bug).

[scordio]

If I understand correctly, you're looking for a way to verify the correctness of a Comparable implementation, similar to what EqualsVerifier does with the equals/hashCode.

Usually, AssertJ doesn't have such responsibilities behind a single assertion method. Plus, I'd consider that users might end up with fancy combinations due to usingComparator.

I was curious to see what the ecosystem offers today and I found jqno/equalsverifier#129 – I guess the world is small after all 🙂

What you're asking would impact all the ComparableAssert methods, if we did it consistently. I'm not yet convinced that we should have such an aggressive change.

@joel-costigliola thoughts?

[xenoterracide]

The big difference between the two of course is that equalsverifier actually creates the object and fills in all the fields and such. I'm just talking about comparing existing instances. It could be done in (I think) no more than three lines of code internally but that multiplies externally to every single comparison that you need to do.

[xenoterracide]

Oh, another thought I had. I'm not testing my own implementation of equals like I would use equalsverifier for. What I'm really doing here It is asserting that the versions I'm generating actually have the precedence that I think they do. Turns out nothing about how these particular classes are interpreting these are how I thought they interpret these.

[joel-costigliola]

My concern if we go that way is that the assertion error could be confusing on the additional hidden assertion because you are switching actual and expected.

With parameterized tests, you can achieve your goal with minimal effort IMHO so overall I'm not convinced of the value of such an evolution.