Document that assert[Not]Same should not be used with primitive types

[martinfrancois]

Steps to reproduce

Run the following Test Case:

assertTrue(127 == 127);    // succeeds
assertSame(127, 127);      // succeeds

assertTrue(128 == 128);    // succeeds
assertSame(128, 128);      // fails

Context

• Used versions (Jupiter/Vintage/Platform): JUnit Jupiter Version 5.9.3
• Build Tool/IDE: IntelliJ IDEA

This is a similar issue to: assertj/assertj#2887

It appears to be the same kind of autoboxing caching issue due to Object being used in assertSame. This also means that, as mentioned in the issue, it not only affects this exact case, but also any case mentioned in JLS section 5.1.7:

If the value p being boxed is an integer literal of type int between -128 and 127 inclusive (§3.10.1), or the boolean literal true or false (§3.10.3), or a character literal between '\u0000' and '\u007f' inclusive (§3.10.4), then let a and b be the results of any two boxing conversions of p. It is always the case that a == b.

This means that for example, the following case is also affected:

assertTrue('ç' == 'ç');  // succeeds
assertSame('ç', 'ç');    // fails

Deliverables

• assertSame(128, 128); passes along with other cases in the JLS section 5.1.7

[sbrannen]

Technically speaking, assertSame(128, 128) works as expected.

The Javadoc clearly states the following:

Assert that expected and actual refer to the same object.

And the error message even shows that there are two distinct Integer objects.

org.opentest4j.AssertionFailedError: expected: java.lang.Integer@7995092a<128> but was: java.lang.Integer@7fc2413d<128>

If we want assertSame(128, 128) to pass, we would have to introduce an overloaded assertSame(int, int) method, and we would also need to introduce overloaded variants of assertSame() for other primitive types.

So the question to answer is whether we want to introduce those overloaded variants.

[martinfrancois]

Technically speaking you are correct, yes. Good point on the error message, I can still imagine developers unaware of the caching would be confused about why two of the seemingly same primitive values result in different references in that case, which could mislead them into thinking their implementation is wrong and a lot of time wasted in debugging.

In the end it comes down to whether JUnit users would expect assertSame to have overloaded variants, as you mentioned. Personally, I couldn't think of any case where a developer would on purpose write code using primitive values where they would make use of the fact that 127 always references the same object, while 128 doesn't. I would consider this to be bad design as it is very implicit, yet this would be the only case I can think of where not having overloaded assertSame methods would be expected.

I could be missing something of course, but I think it would hurt JUnit users more not to have overloaded assertSame methods for primitive types than to have them.

[sbrannen]

I could be missing something of course, but I think it would hurt JUnit users more not to have overloaded assertSame methods for primitive types than to have them.

We'll likely discuss this topic within the team during an upcoming team call; however, if someone invokes assertSame(128, 128), they are effectively using the API wrong.

assertSame() is only intended to check object reference identity. It does not check equality, and in Java it is important to understand the difference between identity and equality.

For equality, we have assertEquals(int, int), and assertEquals(128, 128) passes.

If we do decide to introduce primitive type overloads for assertSame(), each of those overloads would simply delegate to the corresponding assertEquals() method, which seems a bit unnecessary to me. However, I suppose it could be beneficial to inexperienced Java developers who don't yet understand the difference between equality and identity (and who possibly are not aware of Java's support for auto-boxing).

[sbrannen]

As a side note, assertSame(new Integer(127), new Integer(127)) also fails (for hopefully obvious reasons).

[marcphilipp]

assertSame is not intended to be used with primitive types. Can you use assertEquals instead?

[marcphilipp]

Team decision: Document that assertSame should not be used with primitive types.

[martinfrancois]

assertSame() is only intended to check object reference identity. It does not check equality, and in Java it is important to understand the difference between identity and equality.

Agreed. The == operator in Java is often associated with checking for identity and you typically also use it to check for equality between primitive data types. Given equals is exclusively used for checking the equality of two objects and == is typically used to compare identity, I think it's not unexpected for developers who start learning JUnit intuitively to start associating equals with assertEquals and == with assertSame and to always use them as such, even though this technically means you would be using the API wrong, as you were pointing out. Then, as 127 == 127 is true, just like 128 == 128 is, I don't think it's too far off to expect assertSame to work in the same way.

assertSame is not intended to be used with primitive types. Can you use assertEquals instead?

In most cases I would say yes, unless you want to verify a primitive datatype is used, as assertEquals will still pass in case of assertEquals(new Integer(128), new Integer(128)) for example, which is why I tended to use assertSame in those cases in the past.

Team decision: Document that assertSame should not be used with primitive types.

This was one approach I was thinking of as well. One option to take it further could be to throw an UnsupportedOperationException if assertSame is attempted to be used with primitive data types to make sure it is not used in the wrong way.

[sbrannen]

One option to take it further could be to throw an UnsupportedOperationException if assertSame is attempted to be used with primitive data types to make sure it is not used in the wrong way.

That's not possible without introducing overloaded variants for all affected primitive types.

[sbrannen]

I have changed this issue's title to align with the current action item.