Better handling of Array[T] in assertions

[sirthias]

Currently (version 0.7.23) munit doesn't provide special handling for comparing arrays, which has certainly tripped up users many times already.

uTest provides a simple hack, which isn't pretty, but gets the job done.

The suggestion is to provide something similar (or even better) for special-casing arrays.

[olafurpg]

Thank you for reporting! Can you elaborate on the expected behavior? Should assertEquals have a special case for arrays or should there be a separate assertions?

[sirthias]

I think the expected behavior would be to have assertEquals(array1, array2) behave exactly as assertEquals(array1.toSeq, array2.toSeq). Requiring me to use special assertions would defeat the purpose of not having to remember array's special needs with regard to equality.

Of course, it'd be great to have a solution that would also and transparently "to the right thing" (™) for nested arrays located e.g. somewhere in the depths of my case class hierarchy but that would of course raise a number of much more difficult to answer questions.

[olafurpg]

For better or worse, arrays use reference equality so I would be surprised if MUnit had a special case for arrays like that. It’s especially unsatisfying that such a workaround doesn’t handle nested arrays.

There’s an open PR that implements support to configure the equality of assertEquals via a typeclass. It’s a binary breaking change so it’s been kept open for a while now. Is this something you think could address this issue?

[olafurpg]

FWIW, MUnit has special assertions for comparing equality of float/double primitives. I think it would be fine to add something similar for arrays. The benefit of a separate assertion is that it makes (IMO) the code easier to understand when you assert two arrays have structural equality. We shouldn’t hide the fact that arrays have reference equality on the JVM, it’s highly relevant if your application uses arrays.

[sirthias]

Typeclass-based equality would (and will) be the proper, long-term solution, yes.
Currently, however, MUnit doesn't support sensible assertions against arrays anywhere, nested or not.

Quite a few codebases that I've been working in use arrays as simple and fast helper structures in tests and I always trip over the ref-equality special casing.
Right now, during the migration to MUnit from utest, it came up again, since utest transparently supports top-level arrays, while MUnit doesn't.

The benefit of a separate assertion is that it makes (IMO) the code easier to understand when you assert two arrays have structural equality.

Adding a separate assertion without changing the existing ones wouldn't really add much value IMHO.
Reading the assertion assertNotEquals(array1, array2), when would an average user (like myself) expect it to fail? I certainly don't expect it to rely on ref-equality here (as I never do and always forget the special case).
So this test will not do what I think it does and will never fail, which is of course sth I would expect the testing lib to help me prevent!

I won't even remember to use a special, potential assertArrayNoEquals.
If I do I might as well have used assertNotEquals(array1.toSeq, array2.toSeq) in the first place.
The point of this ticket is to prevent assertions from accidentally and unintentionally be used in the wrong way on arrays.

Maybe the solution would be to special case arrays in assertEquals and assertNotEquals and have them always fail with the following error message?

Array equality is reference-based, which might not be want you want. Please use 'assertArrayEquals',
'assertArrayNotEquals', 'assertArrayRefEquals' or 'assertArrayNotRefEquals' to make your intentions clear!

Then accidental errors would become impossible and the tests remains very clear to all readers.

---

I myself currently use this wrapper around MUnit to get the job done. It doesn't support nested arrays but they don't appear in our case class hierarchies anyway, so that's a non-issue for us:

class MySuite extends FunSuite {

  implicit class ArrowAssert[A](lhs: A) {

    @nowarn("cat=unused-params")
    def ==>[B](rhs: B)(implicit ev: MySuite.CanEqual[A, B], loc: munit.Location): Unit = {
      def arrayToSeq(x: Any) =
        x match {
          case a: Array[_] => a.toSeq
          case _           => x
        }
      assertEquals(arrayToSeq(lhs), arrayToSeq(rhs))
    }
  }
}

object MySuite {

  @implicitNotFound("Types ${A} and ${B} cannot be compared]")
  sealed trait CanEqual[A, B]

  object CanEqual extends LowerPrioCanEqual {
    implicit def canEqual1[A, B <: A]: CanEqual[A, B] = null // phantom type
  }

  abstract class LowerPrioCanEqual {
    implicit def canEqual2[A, B <: A]: CanEqual[B, A] = null // phantom type
  }
}

[sirthias]

As another data point: zio-test, just like uTest, also comes with special handling for Array equality built in:
https://github.com/zio/zio/blob/master/test/shared/src/main/scala-2.x/zio/test/AssertionVariants.scala#L29

[danicheg]

I totally agree with @sirthias that MUnit should either have assertion for Arrays, either assertEquals should have specific logic for Arrays. I've done a third variant #393 - assertion that could check elements of Iterables. But for me, it's ok if assertEquals would have specific on checking elements of Arrays and not check refs (is it have a sense for Arrays on the whole?).

[SethTisue]

When there is something unfortunate in Scala like Arrays unusualness wrt equality, I think MUnit shouldn't paper over it.

Array being different is something that Scala programmers need to know. Making MUnit handle it differently doesn't reduce overall cognitive load for users, because as a Scala programmer I still need to know how Array works regardless (because Array equality could arise anywhere in any Scala code), but now I need to learn an additional thing, which is that MUnit handles it differently.

I hope MUnit will choose to simply leave well enough alone here. Giving existing methods special behavior isn't an improvement IMO, and adding more methods I have to learn isn't an improvement either.

Keep MUnit simple and minimal!

And if it is decided that this must be addressed, then please address it by adding new methods, rather than by silently giving an existing method special semantics that differ from Scala's usual semantics.

(I'm agreeing with Olaf's previous remarks.)

[sirthias]

When there is something unfortunate in Scala like Arrays unusualness wrt equality, I think MUnit shouldn't paper over it.

Array being different is something that Scala programmers need to know. Making MUnit handle it differently doesn't reduce overall cognitive load for users, because as a Scala programmer I still need to know how Array works regardless (because Array equality could arise anywhere in any Scala code), but now I need to learn an additional thing, which is that MUnit handles it differently.

I can completely subscribe to this take.
However, the problems and risks of assertions being accidentally and unintentionally used in the wrong way on arrays is real and IMHO our tools should support us where they can rather than take the stance of C and simply requiring us developers to "be careful" and "not make mistakes".
If a testing lib can help me with remembering the special equality status of arrays then I would like it to do so rather than simply push all the responsibility to me.

And if it is decided that this must be addressed, then please address it by adding new methods, rather than by silently giving an existing method special semantics that differ from Scala's usual semantics.

Yes, having thought more about this issue I now think that the best way for MUnit would be to improve upon uTest and zio-test (rather than mimicking them) by adding special methods for arrays and produce errors somewhat like these (quote from above) for the "normal" ones:

Array equality is reference-based, which might not be want you want. Please use 'assertArrayEquals',
'assertArrayNotEquals', 'assertArrayRefEquals' or 'assertArrayNotRefEquals' to make your intentions clear!

[SethTisue]

If a testing lib can help me with remembering the special equality status of arrays then I would like it to do so rather than simply push all the responsibility to me.

Perhaps this could be accomplished via a Scalafix linting rule rather than through API expansion?

[sirthias]

Yes, MUnit could simply push this to Scalafix.
However, then a ticket like this would still reappear here every so often, because the user sets of MUnit and Scalafix will only ever overlap partially. Also, I'd say that making misuse impossible is preferable to softer solutions if the cost is as low as here.

Additionally, Scalafix wouldn't be able to fix the problem on the code reading side.
In the middle of a code review I still have to be very careful to mentally assign the special "array" status to a val used in a test to spot potential "ref-equality" bugs.
Making the API crystal clear would all but eliminate the misuse-risk completely and let me focus fully on the domain part of the code, which is what I want to do.
Adding a few special assertions for arrays doesn't feel like complicating the API at all.
In the contrary, since these would make MUnit easier to use correctly I'd say they reduce the mental load required for me, since I'd have to keep fewer things in my head ("no worries about arrays, MUnit protects me from any misuse").
Also, with sth like the error message above this part of the API would be entirely discoverable without any additional documentation.

[olafurpg]

I opened #395 to customise the error message when comparing arrays with equal values. Thank you @SethTisue for sharing your thoughts. I agree that MUnit should not hide the fact that arrays use reference equality (which isn't even an unfortunate design decision IMO, it's a reasonable choice for mutable collections). Users can implement their own custom assertions (or even override assertEquals) if they don't like default behavior.