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

AbstractOptionalDoubleAssert.hasValue(double) fails with NaN #3401

Closed
Caceresenzo opened this issue Mar 15, 2024 · 11 comments
Closed

AbstractOptionalDoubleAssert.hasValue(double) fails with NaN #3401

Caceresenzo opened this issue Mar 15, 2024 · 11 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@Caceresenzo
Copy link

Caceresenzo commented Mar 15, 2024

Describe the bug

The AbstractOptionalDoubleAssert.hasValue(double) javadoc state that NaN values are supported (last line of):

* Assertion will pass :
* <pre><code class='java'> assertThat(OptionalDouble.of(8.0)).hasValue(8.0);
* assertThat(OptionalDouble.of(8.0)).hasValue(Double.valueOf(8.0));
* assertThat(OptionalDouble.of(Double.NaN)).hasValue(Double.NaN); </code></pre>

The nature of NaN values prevents the == operator to works, but there is no hasNaNValue() method or equivalent, and the javadoc clearly state that it should works.

  • assertj core version: 3.24.2
  • java version: 21
  • test framework version: junit 5.10.1
  • os (if relevant): windows

Test case reproducing the bug

@Test
void testNaN() {
	assertThat(OptionalDouble.of(Double.NaN)).hasValue(Double.NaN);
}

Result:

org.opentest4j.AssertionFailedError: 
Expecting actual:
  OptionalDouble[NaN]
to contain:
  NaN
but did not.
@Caceresenzo
Copy link
Author

In the meantime, that can be bypassed by testing the double value directly:

@Test
void testNaN() {
	assertThat(OptionalDouble.of(Double.NaN).getAsDouble()).isNaN();
}

@scordio
Copy link
Member

scordio commented Mar 15, 2024

Thanks for reporting it, @Caceresenzo!

@scordio scordio added the type: bug A general bug label Mar 15, 2024
@scordio scordio added this to the 3.26.0 milestone Mar 15, 2024
@pbacz
Copy link
Contributor

pbacz commented Mar 21, 2024

@scordio Do you need someone to make a change for this?

@scordio
Copy link
Member

scordio commented Mar 22, 2024

Sure, go for it, @pbacz!

@pbacz
Copy link
Contributor

pbacz commented Mar 25, 2024

@scordio
The simplest solution here would be to replace == comparison with equals() comparison.
This will have a side effect causing any test that compares 0.0 to -0.0 to fail (currently passing).

From the javadoc stating that assertThat(OptionalDouble.of(Double.NaN)).hasValue(Double.NaN) should pass I conclude that this was always the intent to compare using equals().

I propose to replace == check with equals() check and also state clearly in the javadoc which is used.

As this is potentially a breaking change for an edge case I'd like you to confirm if I should proceed with that.

@Caceresenzo
Copy link
Author

Caceresenzo commented Mar 25, 2024

A solution could be to add isNaN() to match AbstractDoubleAssert's API. (and add isNotNaN() too?)

This would prevent breaking changes.

EDIT: typos

@pbacz
Copy link
Contributor

pbacz commented Mar 26, 2024

@Caceresenzo @scordio
Sure, I can do it either way. All I need is a decision

@Caceresenzo
Copy link
Author

Caceresenzo commented Mar 27, 2024

I think having both isNaN() and isNotNaN() would be best as there do not seems to be a method for the opposite of hasValue(...).

Currently this passes:

assertThat(OptionalDouble.of(Double.NaN))
	.isNotEqualTo(Double.NaN);

(but I am sure its because its testing against the OptionalDouble and not the optional's value itself)

@scordio
Copy link
Member

scordio commented Mar 28, 2024

I propose to replace == check with equals() check and also state clearly in the javadoc which is used.

I tend to favor this solution, consistent with what the Javadoc already states and also in line with OptionalDouble::equals.

It's indeed a breaking change and the workaround to get the previous behavior would be something like:

assertThat(OptionalDouble.of(Double.NaN).getAsDouble()).isSameAs(Double.NaN);

@joel-costigliola do you agree?

@pbacz
Copy link
Contributor

pbacz commented Apr 2, 2024

@scordio @joel-costigliola
I created a PR for this issue.

@joel-costigliola
Copy link
Member

Integrated thanks @pbacz !

joel-costigliola pushed a commit that referenced this issue May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants