Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix assertSame to match semantics of == for primitive types #1523

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gb96
Copy link

@gb96 gb96 commented May 24, 2018

This PR attempts to address some potentially unexpected behaviour of assertSame() when passed primitive expected and actual arguments.

Currently:

  1. assertSame(123, 123) passes because the although the primitive int arguments are boxed into Integer, most implementations of JVM cache the boxed Integers for values of 0 through 127.
  2. assertSame(130, 130) fails because the primitive int arguments are boxed into 2 distinct instances of Integer.
  3. assertSame(true, true) and assertSame(false, false) both pass but only because JVM implementations typically box boolean values to the same 2 constant instances Boolean.TRUE and Boolean.FALSE.
  4. assertSame() for the other primitive types (char, byte, short, long, float and double) will likewise succeed or fail on the basis of whether the JVM boxing implementation decides to resolve the two arguments to the same reference or not.

Although some might argue that developers simply avoid using assertSame() with primitive types, plenty of API returns primitive values, and if assertSame() is used then the principle of "least surprise" should apply.

The solution implemented in this PR follows the rationale that:

  • assertEquals(a, b) should correspond to the semantics of a.equals(b) for distinct objects or boxed primitives, with the addition of corner cases for one or both arguments being null
  • assertSame(a, b) should correspond to the semantics of a == b for both Objects and primitive values.

I initially considered whether assertSame(a, b) might correspond to the (JavaScript / ECMA) semantics of a === b, (i.e. both type and value the same) however that would be quite laborious to implement due to implicit Method Invocation Conversions.

I have implemented new assertSame() functions with signatures taking boolean, long, and double primitive arguments. The aforementioned implicit Method Invocation Conversions take care of the remaining primitive types.

I realize the new assertSame(boolean, boolean) is redundant owing to the fact that assertSame(Object, Object) happens to work as expected for boxed boolean values, however it is there for consistency. If all other primitive types are spared auto-boxing when passed to assertSame() why should boolean be left out?

@Tibor17
Copy link
Contributor

Tibor17 commented Mar 23, 2019

@marcphilipp
@kcooney
Can add this improvement in 4.13?
This change avoids passing NULL in assertions and avoids combinations like NULL and non-NULL.

@marcphilipp marcphilipp changed the base branch from master to main June 21, 2020 17:05
@wangminpku
Copy link

Dear gb96(@gb96 )and Junit team (@kcooney)(@marcphilipp )(@stefanbirkner ) (@Tibor17 )

We are working on automatically identifying and clustering the related pull requests for code review. We have found that PR#1533, PR#1300, PR#992 and PR#632 might be related to this one, because both of them focus on the same problem.

We would really appreciate it if you could help us to validate and give us some feedback if the information from PR#1533, PR#1300, PR#992 and PR#632 is helpful when you review the current PR.

Thank you very much for your time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants