search

CesiumGS / cesium-native

Apache License 2.0
402 stars 205 forks source link

Fix unit tests failing for macOS (arm64) #866

Closed csciguy8 closed 3 months ago

csciguy8 commented 3 months ago

Fixes macOS build failure in latest merge to main, here.

Also fixes any other macOS builds that happen to use a runner configured with Image: macos-14-arm64.

Configure runner for macos-12 explicitly, instead of macos-latest. When choosing the latest, it could actually choose an arm64 variant, which our unit tests fail on (at least 3 related to narrowing). In addition, even specifying macos-14 can lead to macos-14-arm64 being chosen.

After review, and we go this direction, we should write an issue to support the latest builds (14, arm64) on CI.

csciguy8 commented 3 months ago

Tagging @kring for a review, since we discussed this a bit already.

Also, would be nice to get this fix in sooner than later. Kind of annoying to see this error popup on everyone's branches.

kring commented 3 months ago

So @csciguy8, in the particular example in the test, is the Apple Silicon CPU actually able to convert the value to a double and back while preserving the value exactly? While the Intel CPU is not? If so, changing or removing the test is reasonable; it's just a bad test.

However, if the value is not preserved, and yet an exception isn't thrown, then the function is not fulfilling its contract. In that case it's a bug and we're hiding the bug by removing the test.

This may seem a little academic, but it matters. This cast (the non-throwing version, I believe) is used in the metadata system to determine which metadata types can be used to represent a set of values. If cesium-native thinks a large Int64 can be represented as a double, when it actually can't, then we will end up silently introducing error into users metadata values, which is among the worst kinds of bugs we can have.

kring commented 3 months ago

My response above is to the original version of this PR, where you removed the test. Switching back to macos-12 temporarily to unbreak our builds is reasonable. Merging.

csciguy8 commented 3 months ago

@kring Agree with your thoughts.

I originally went the direction of removing the test, then saw that two more were failing, then decided looking into this more was probably the best way to go.