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

Make TestCounterAddLarge more robust #567

Merged
merged 1 commit into from
May 4, 2019
Merged

Make TestCounterAddLarge more robust #567

merged 1 commit into from
May 4, 2019

Conversation

beorn7
Copy link
Member

@beorn7 beorn7 commented May 3, 2019

The previous float64(math.MaxUint64 + 1) is too close to
float64(math.MaxUint64) to actually overflow as indended.

The counter code is actually converting forward and backward and
compare the original and twice-converted value. On most platform, this
will create a deviation and thus trigger the expected behavior. By
sheer "luck", one might end up with the same value and thus still use
the uint64 representation. Which is OK within the precision we can
expect. But it breaks the test. With this change, the next
representable floating point value greater than the floating point
value used to represent math.MaxUint64 is used.

@xnox this should fix #530. Could you verify?
@TheTincho you might have seen this issue, too, for the Debian builds on the affected platforms.

The previous `float64(math.MaxUint64 + 1)` is too close to
`float64(math.MaxUint64)` to actually overflow as indended.

The counter code is actually converting forward and backward and
compare the original and twice-converted value. On most platform, this
will create a deviation and thus trigger the expected behavior. By
sheer "luck", one might end up with the same value and thus still use
the uint64 representation. Which is OK within the precision we can
expect. But it breaks the test. With this change, the next
representable floating point value greater than the floating point
value used to represent math.MaxUint64 is used.

Signed-off-by: Bjoern Rabenstein <bjoern@rabenste.in>
@brian-brazil
Copy link
Contributor

👍

@beorn7
Copy link
Member Author

beorn7 commented May 4, 2019

Thanks @brian-brazil . This change makes things saner in any case. @xnox please follow up on this (or #530) if your tests are still failing.

@beorn7 beorn7 merged commit 906d297 into master May 4, 2019
@beorn7 beorn7 deleted the beorn7/counter branch May 4, 2019 16:08
@xnox
Copy link

xnox commented May 7, 2019

With this patch applied, build passed on arm64 ppc64el s390x. Cherrypicked into Ubuntu and uploaded.

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.

TestCounterAddLarge fails on some architectures
3 participants