willowtreeapps/assertk

Collection oriented assertion should respect the underlying collection behavior.

andriesfc opened this issue · 1 comments

andriesfc commented

I've started to use this library in one my projects as my preferred choice of assertions. But I notice the family of containsXXX() assertions just have to many caveats associated with it.

To wit:

  1. As stated in issue #2:

    right now it's containsAll(vararg Any?) which it makes it too easy to accidentally pass in listOf(foo) instead of *listOf(foo)

  2. This confusion can be extended to assertions such as containsOnly(), containsExtactly()

  3. The behavior of containsOnly is especially problematic in the following way: You have to remember the following incantation if you want to assert using a collection:

    containsOnly(*someCollection.toTypedArray())

I believe the root of the problem can be reduced, (as far as implementation), to the following issues:

  1. The current API makes the implicit assumption in these cases that values passed to the assertions should behave like an array structure.
  2. As such, arrays have certain underlying properties which may be different than the expected values a user may assert against. As an example, consider, the Set<T> type, note the following:
    • Sets are not allowed to have duplicates.
    • Sets may have not any order (even from one call to another)
    • A set may be ordered, either by natural order, or by some other property. (One can argue they are not even true sets anymore, but should be called Unique Collections instead)

I propose the following:

  1. Introducing a dedicated family of assertions for each distinct common collection like type:

    • List<T>

      • containsList<T>(expectedList)
      • containsSubList<T>(expectedSubList)
      • containsSomeOfList(possibleListElements)
      • doesNotContainList<T>(ommittedList)
      • doesNotContainSublist(ommittedSubList)
      • doesNotContainAnyOfList(listOfOmmittedElements)
      • hasOrderOf(comparator)

    • Set<T>

      • containsSet<T>(expectedSet)
      • containsSubSet<T>(expectedSubSet)
      • containsAnyOfSet<T>(possibleSetOfElements)
      • doesNotContainSet<T>(ommittedSet)
      • doesNotContainSubSet<T>(ommittedSubSet)
      • doesNotContainsSomeOfSet<T>(possibleSetOfOmmisions)

    • SortedSet<T> extends the above assertions to include the following:

      • containsSetInOrder(orderedSet)
      • doesNotContainSetInOrder(orderedSet)

        ⚠ Here we assert the actual set contains the set, but not in expected order!

      • hasOrderOf(comparator)

        Introducing the ability to check for a certain order.

    • Collection<T>

      • containsCollection(collection)
      • containsSomeOfCollection(possibleInclusions)
      • containsAnyOfCollection(possibleInclusions)

    • Map<T>

  2. Introducing a dedicated family of assertions against the Array<T> type, keeping with the theme of using varargs:

    • containsArray(vararg)
    • containsSomeInArrayOf(possibleInclusions)
    • doesNotContainAnyInArrayOf(possibleOmmisions)

  3. Keep the current API, but mark it as deprecated to give everybody a change to move/update their unit tests.

This proposal offers substantial benefits:

  1. First and foremost, it shifts the burden of selecting the appropriate collection type to the user.
  2. It is very explicit and descriptive. Which makes excellent readable unit tests.
  3. It avoids the use of non-intuitive incantations such (* colletion.toTypeArray()) entirely. A user may opt use this incantation as part of the actual business logic in their own test.
  4. Such an implementation respects the actual underlying data type constraints.
  5. Lastly, I believe that it will improve the implementation of additional assertions by not introducing subtle edge cases, such as point 3.

There are some risks to this proposal which should be noted:

  • Implementing this proposal will consume a substantial amount time.
  • Implementing this proposal will increase the surface of both the public and internal API, (and with it the maintenance burden).
  • It will defiantly increase the size of the assertk.support package, as a lot of commonalities from this proposal can and should still be extracted.
  • The removal of the old, deprecated assertions will be a breaking change. But restricting such removals to even major version updates, (or even every second even major version update), should allow the proposal to move forward without causing undue friction to any users.

Lastly, on a personal note. I would like to offer my time to work on this. Using this library has been such a joy (in contrast of JUnit5 which does not cater for Kotlin as first-class concern).

I do not see this proposal as complete, or without fault. And would appreciate any feedback either way.

charlesritchea commented

This gets me every time I try to use containsXXX

  • 👍1