Skip to content
New issue

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

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

Already on GitHub? # to your account

Add NaN function #691

Closed
wants to merge 1 commit into from
Closed

Add NaN function #691

wants to merge 1 commit into from

Conversation

yahelmanor
Copy link

Adding NaN to assert.
Adding tests for the NaN function.

resolve #624 .

Adding NaN to assert and test for it.
@georgelesica-wf
Copy link

I'm hesitant to expand the API for this since it is a pretty niche use-case that is already covered pretty cleanly, as noted in the issue discussion: assert.True(t, math.IsNaN(i)).

The way I see it, there are two benefits to having a special assertion for a given type or situation:

  1. Brevity / conciseness / readability
  2. Useful failure messages

In this case, the alternative is nearly as concise, so we need to think about (2). In this case, the failure message would include the value that was actually found, which might be helpful. But if you're expecting NaN, how likely is the actual value to matter? I would think that the operands used to produce the value would be more useful, but we can't automatically include those in the message since we don't know them. Therefore, users will still, likely, end up writing custom failure messages.

So I'm just not sure that this is helpful enough to warrant its own assertion, particularly given that I wouldn't expect this assertion to be used frequently.

I'm happy to discuss it further, however.

@kylebrandt
Copy link

@georgelesica-wf I think the case that gets messy is when you have NaNs inside a data structure. go-cmp takes some options on how to compare things which is pretty nice, see (https://godoc.org/github.com/google/go-cmp/cmp#example-Option--EqualNaNs).

This allows for basic Object in / Object out table tests when working with any data structure that has NaN values in it.

@dolmen
Copy link
Collaborator

dolmen commented Mar 6, 2024

assert.True(t, math.IsNaN(i)) is good enough.

@dolmen dolmen closed this Mar 6, 2024
@dolmen dolmen added wontfix pkg-assert Change related to package testify/assert and removed on hold labels Mar 6, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement pkg-assert Change related to package testify/assert wontfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

assert two NaN values
4 participants