Wrong order of arguments for assertEquals

[OlivierBlanvillain]

In junit, assertEquals is defined as assertEquals(expected, actual), but in munit, it's the other way around.

It's really unfortunate because the type constraint on assertEquals will sometimes require the arguments to be swapped, for instance when asserrting that something is Nil or None, meaning this not just a documentation issue, but a migration burden...

I know this is probably too late to fix because it would mess up everyone's munit tests, so just close it and give me the most annoying issue award 😛

[olafurpg]

Thank you for reporting! This has been brought up before and I agree it's unfortunate. The current choice of the parameter ordering is not random, it's motivated by a particular pattern I use a lot in my own codebases, which is to write assertions where the expected value is a long multi-line string

assertNoDiff(
  someMethod(),
  """|Hello
     |World!
     |""".stripMargin
)

The expected value sometimes spans 10+ lines while the obtained/actual value is often just a small expression. The code would be more difficult to read if we swapped the ordering.

I am afraid it's unlikely this will change. The only workaround would be to roll your own munit.Suite with a different set of assertions. You could mix in a custom implementation of Assertions like this

munit/munit/shared/src/main/scala/munit/FunSuite.scala

Line 13 in af3931c

with Assertions

However, we may need to do some extra work to avoid the this: FunSuite => constraint on other traits like ValueTransforms.

[SethTisue]

I can see there are tests that benefit slightly from the actual-first ordering, but this pattern benefits from the expected-first ordering:

assertEquals(expected, {
  lots of
  code here
  stretching out
  vertically
})

Personally I think this is considerably more common than the "long expected" string case, though it would be hard to prove that.

In any case, either pattern is easy enough to accommodate with an extra local val. When I have an long-expected-string test I can write:

val expected = long
                          multiline
                          thing
assertEquals(expected, actual)

Given how easy that is to do either way, I really think it's very bad for MUnit not to follow the JUnit precedent, if you want MUnit to be widely used. The gain of using a nonstandard ordering is so small, and the cost is so large: endless mental friction.

[olafurpg]

Stability is one of my key motivations for creating in MUnit. I'm hoping to release v1 before the end of the year and never introduce breaking changes after that.

While I sympathize with your perspective, I struggle to justify introducing such a breaking change to 100% of existing MUnit users in favor or making the migration easier to hypothetical new users coming from JUnit assertions. ScalaTest and utest assert(a == b) are macros that are agnostic to the ordering of parameters. Minitest assertEquals uses the same ordering as MUnit (https://github.com/monix/minitest/blob/f1d4174e9c60bcf75740bcfb531b8de8f8c793c5/shared/src/main/scala/minitest/api/Asserts.scala#L65).

[gabro]

I'll add to the discussion, saying that this is a problem of migrating from JUnit to MUnit, which (while annoying in some instances where the order matters) is still a fairly rare use case.

I don't think many projects in Scala other than the compiler itself use JUnit directly, so I don't think it would make sense to introduce a huge breaking change to support that specific use case.

For MUnit to get larger adoption, I think it would rather make sense to focus on making Scalatest / Speca2 migration path easier, but that's an entirely different topic

[OlivierBlanvillain]

I struggle to justify introducing such a breaking change to 100% of existing MUnit users in favor or making the migration easier to hypothetical new users coming from JUnit assertions.

That's completely fair. However, I think a possible middle ground would be to remove the <:< constrain on assertEquals which would at least make it possible to quickly migrate for lazy people like me that would rather live with broken sementics in their test suite than waste time swapping argument of assertEquals.

To up this in perspective, it took me ~4 hours to migrate EPFL's moocs repo (~40k loc, half tests) from JUnit to MUnit, and about half of that time was spent messing around with assertEquals.

I'm all in favor of compile time safety, but in the precise case of assertEquals I think it redondent given that if someone messes up the argument, for instance by putting the clue first like in JUnit, the test will fail at runtime anyway...

[olafurpg]

I can imagine that must have been an annoying migration. I took a stab in #191 at implementing a basic Scalafix migration rule that swaps the arguments for JUnit assertEquals(expected, actual). I ran the rule on the scala-js repo and got the following diff, which looks reasonable https://github.com/scala-js/scala-js/compare/master...olafurpg:munit?expand=1 (see second commit message for steps to reproduce).

I'm all in favor of compile time safety, but in the precise case of assertEquals I think it redondent given that if someone messes up the argument, for instance by putting the clue first like in JUnit, the test will fail at runtime anyway...

Did you try to use assertEquals[Any, Any] as a workaround? I was also leaning towards removing the type parameters because they appeared to be causing more harm than good: complicated signature, false negatives, runtime tests fail anyways. However, after @gabro discovered the assertEquals[Any, Any] workaround I haven't been so much bothered by this anymore. Every time I get a compile error from assertEquals it usually help me catch a bad smell in my code.

[OlivierBlanvillain]

well, with Product with Serializable,
sometimes it's not a code smell, it's just Scala, with Product with Serializable

Jokes apart, this is educational material so I really tried to avoid [Any, Any] annotations 😄

[gabro]

sometimes it's not a code smell, it's just Scala, with Product with Serializable

Right, it's a hard balance to strike, but I think it's really nice (even more so for educational purposes) that you get a compiler error for stuff like

val x = Some(2)
val y = 4

assertEquals(x, y) // woops

I make many of those little mistakes while writing tests and so far it's been really nice for those to be caught by the compiler

[olafurpg]

Jokes apart, this is educational material so I really tried to avoid [Any, Any] annotations 😄

If this is for education material then I agree it would be better to remove the type parameters. The best argument IMO to remove the type parameters is that it makes the library less accessible for beginners, who will struggle to understand the <:< incantations in the signature. It's difficult to satisfy all needs 🤷

[gabro]

The best argument IMO to remove the type parameters is that it makes the library less accessible for beginners

At the same time, it may be quite surprising for a beginner that a strongly typed language like Scala can't detect a simple mistake like the one I've shown above.

[olafurpg]

@gabro i think it’s more likely that students become frustrated by the cryptic error message that don’t explain how to fix the problem. The current API is pretty much designed for power users.

[smarter]

I make many of those little mistakes while writing tests and so far it's been really nice for those to be caught by the compiler

Another way to deal with these in Dotty is to use the Eql typeclass: http://dotty.epfl.ch/docs/reference/contextual/multiversal-equality.html

[olafurpg]

I agree. I’ve been considering whether to use the Eql typeclass in Dotty only.

[olafurpg]

I opened #223 to start using Eql in Scala 3 and to drop the subtyping constraint in Scala 2. This means it's no longer a compile error to compare unrelated types unless you're using Scala 3 with strict equality enabled

[som-snytt]

As another data point, I prefer p = assertEquals(expected) as a predicate. Then p(actual) to test or assertEquals(exp)(sut).

It's an interesting discussion, whether it's more LOC to construct the expected or execute the SUT.

I may contribute it as an alternative flavor and see if it catches on; or just continue to use the idiom privately.

[som-snytt]

TIL olafurpg's paradigmatic example is compileErrors

assertNoDiff(
  compileErrors("Set(2, 1).sorted"),
     """|error: value sorted is not a member of scala.collection.immutable.Set[Int]
        |Set(2, 1).sorted
        |          ^
        |""".stripMargin
)

[olafurpg]

Gonna close this issue as "won't fix". The PR #225 is still open because I wanted to avoid breaking changes during the Scala 3 milestone phase. I think it's a good idea to relax the type constraints on assertEquals so that either parameter can be a subtype of the other (currently, the left parameter must be a subtype of the right parameter). However, this is a binary breaking change so it's something we should try to bundle together with other binary breaking changes as part of the v1 release.

[frankbe]

Even though this is a closed issue now, I would like to encourage you to never make a breaking change in assertEquals and instead create a new method (e.g. named "assertEq") with swapped actual and expected parameters and then disable assertEquals one day.

[som-snytt]

assertEq would be assertSame, but the swapped version could be assertSlauqe pronounced slawk.