How to check that a collection is sorted

[SerVB]

Hi! In assertj, I have .isSorted and .isSortedAccordingTo(comparator). Is there such a thing in Atrium?

[robstoll]

Hi @SerVB thanks for your question. No Atrium doesn't provide such methods so far but we could add it if you think it is something others could have use for as well.

Until Atrium provides it out of the box (with nice error reporting and all) you can add the following extension functions:

inline fun <T : Iterable<E>, reified E : Comparable<E>> Expect<T>.toBeSorted(): Expect<T> =
    this.toContainExactlyElementsOf(
        this._logic.maybeSubject.map { it.sorted() }.getOrElse { emptyList() }
    )

inline fun <T : Iterable<E>, reified E> Expect<T>.toBeSortedAccordingTo(comparator: Comparator<E>): Expect<T> =
    this.toContainExactlyElementsOf(
        this._logic.maybeSubject.map { it.sortedWith(comparator) }.getOrElse { emptyList() }
    )

A few questions back:

1. Can you give it a go and tell me what error reporting you would expect for such functions in case you don't like the output of the above.
2. Also, I suggest we consider an empty list as sorted, do you agree?
3. What do you think if we add also the following in addition:

   inline fun <T : Iterable<E>, reified E,  R : Comparable<R>> Expect<T>.toBeSortedBy(crossinline selector: (E) -> R?): Expect<T> =
   this.toContainExactlyElementsOf(
       this._logic.maybeSubject.map { it.sortedBy(selector) }.getOrElse { emptyList() }
   )
   

[SerVB]

Thanks!

1. Comparing with .sorted is a bit softer check than I expect: .sorted can work even with a broken comparator. What do you think about comparing every element with every other one? It will increase complexity but will be the most strict check possible, I believe.
2. Yep, empty is sorted. It's a great question, actually. Probably we should name the methods like toBeEmptyOrSorted instead to make it more clear. And to provide another set of methods like toContainElementsAndBeSorted, like I see Atrium already has similar methods.
3. Yep, could be useful, to make it on par with Kotlin.

[robstoll]

Can you elaborate on the first point please. sorted works only if the element of the Iterable is Comparable right, there is no comparator involved at all. Thus I am not sure what you mean.

[SerVB]

Oh, I meant .sortedWith. And for .sorted, yep, the comparable can be implemented incorrectly but .sorted of such elements can still return a correct result of sorting as if the comparable were implemented correctly.

So probably iterable.toList() == iterable.sorted().toList() is a soft check. A stricter check would be to check every element in iterable.toList(): is it greater than the ones on the left of it, is it less than the ones on the right of it, and is it equal compared to itself.

It would be N^2 complexity, but as there would be more checks, there would be a bigger chance of noticing an incorrect comparison implementation.

[robstoll]

I can see that the toContainExactly approach will be buggy in case someone implements the Comparable in a wrong way and there are multiple different elements (in the sense of equals) which are the same in terms of Comparable. However, I ask myself if providing something like toBeSorted really helps here because it would be based on the wrong implemented Comparable. Ergo, either Comparable was implemented correctly in terms of equals and the toContainExactly approach is fine, or you need to actually test the comparable implementation as such in which case your expectation should not be based on it.
But maybe I still don’t understand what you mean. could you make a code example please

[SerVB]

you need to actually test the comparable implementation as such in which case your expectation should not be based on it

Well, I meant a case when someone tests comparable using a predefined sorted list, like this:

@Test fun `test MyType comparable implementation`() {
  val actual = listOf(MyType(1), MyType(2), MyType(3))  // can be a much longer list to make more sense
  expect(actual).isSorted()
}

But probably it's not a good practice. Ideally, it's worth testing only specific comparisons to test a comparable implementation. However, it's so easy and tempting to just write a long list of values in order, and then just test that it's sorted...

So probably it's better to not just compare actual == actual.sorted() inside .isSorted(), but to compare all of the following:

• MyType(1).compareTo(MyType(1)) == 0;
• MyType(1) <= MyType(2);
• MyType(1) <= MyType(3);
• MyType(2) >= MyType(1);
• MyType(2).compareTo(MyType(2)) == 0;
• MyType(2) <= MyType(3);
• MyType(3) >= MyType(1);
• MyType(3) >= MyType(2);
• MyType(3).compareTo(MyType(3)) == 0.

This would make the check stricter.

[robstoll]

I see, you take another approach than Atrium (and also assertJ IMO) is made for.
The subject of the expectation (i.e. expect(x) => x = subject) should be the result of an operation and not the values you expect.
I think you should take a data driven approach to test your comparable and not only rely on a long list. Much easier to understand your tests and also covers more checks would be something like:

@Test fun `test MyType comparable implementation`() {
  listOf(
    Triple(MyType(1), MyType(2), -1)
    Triple(MyType(2), MyType(2), 0),
    Triple(MyType(2), MyType(3), 1)
  ).forEach { (first, second, compareToResult) ->
    if (compareToResult < 0) {
      expect(first) {
        toBeLessThan(second)
        notToEqual(second)
      }
      expect(second) {
        toBeGreaterThan(first)
        notToEqual(first)
      }
    } else if (compareToResult == 0) {
       ...
    } else {
       ....
    }
  }
}

In theory you don't even need Triple(MyType(2), MyType(3), 1) as we already cover it with the first entry but I guess MyType can be more complex and you want to test special cases.

With the above you make sure your Comparable implementation aligns with equals.
In the light of this, I think we don't need a toBeSorted. What do you think?

[SerVB]

Well, we've decided to go with another assertions library...

What you say makes sense, but the testing code turns out to be much more complex and explicit. Probably it's a right thing to do, though...

I think we don't need a toBeSorted

Probably it's not worth implementing unless a real user of atrium wants it. I'm not a user, as I've just mentioned.