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

feat: unit test #14119

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: unit test #14119

wants to merge 4 commits into from

Conversation

opensourcevk
Copy link

unit test for utility

@beorn7
Copy link
Member

beorn7 commented May 21, 2024

Thanks for your contribution.

However, I'm not sure if we really need a "test for the test". The code we use here is well established and not expected to ever change again.

@bboreham since you touched this in your refactoring, maybe you have an opinion?

@bboreham
Copy link
Member

This package is used in Prometheus non-test code; I think it’s fine to check it stand-alone.

I have not studied the values used to see if they cover most corner cases.

@bboreham
Copy link
Member

Needs DCO sign-off before we could merge.

@beorn7
Copy link
Member

beorn7 commented May 22, 2024

https://floating-point-gui.de/errors/NearlyEqualsTest.java gives some inspiration for corner cases (with the notable exception that our function returns true if it compares two NaNs (which BTW should probably be mentioned in its doc comment.

@opensourcevk could you do the following:

  • Add a few more corner cases, those not yet covered from the link above.
  • Add a doc comment to the Equal function that it considers all NaNs to be equal to each other.
  • Squash your commits into one, give it a more descriptive description, and sign the DCO.

Thank you very much.

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.

None yet

3 participants