search

google / starlark-go

Starlark in Go: the Starlark configuration language, implemented in Go
BSD 3-Clause "New" or "Revised" License
2.26k stars 204 forks source link

Fix 'math' tests to tolerate float imprecision #409

Closed bdd closed 1 year ago

bdd commented 2 years ago

Came across this while fixing broken Starlark builds for Nixpkgs. It was already market broken for linux-aarch64 and failing for darwin-aarch64.

=== RUN   TestExecFile
    starlarktest.go:117: Traceback (most recent call last):
          testdata/math.star:258:10: in <toplevel>
          builtins/assert.star:15:14: in _eq
        Error: 0.9999999999999998 != 1
    starlarktest.go:117: Traceback (most recent call last):
          testdata/math.star:259:10: in <toplevel>
          builtins/assert.star:15:14: in _eq
        Error: -0.9999999999999998 != -1

I was reproducible with Go 1.16, 1.17, and 1.18 on both Linux and macOS with math.Tan(math.Pi / 4) as well. On amd64, it's consistently 1 but on arm64 it's consistently 0.9999999999999998.

testdata/math.star already provides a near helper for these floating point approximation issues.

This PR modifies two lines two fix the tests on arm64.

google-cla[bot] commented 2 years ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

vcunat commented 2 years ago

I'm no lawyer, but I'm convinced you don't really need CLA or other copyright stuff for such tiny contributions.

adonovan commented 2 years ago

I'm no lawyer, but I'm convinced you don't really need CLA or other copyright stuff for such tiny contributions.

Nor am I, but the CLA policy (written by Google's lawyers) seems to grant an exception only for changes to the copyright headers, but not for changes below some size threshold. Sorry.

FWIW: @bdd, I see you work at Facebook, which has signed the CLA as a corporation, so commits from facebook.com e-mail addresses won't face this extra hurdle.

bdd commented 1 year ago

Gentle ping.

adonovan commented 1 year ago

Sorry for the delay---I thought I was waiting for the CLA check to pass, but it already had.

Thanks for the fix.