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

edge_distances: fix test #106

Merged
merged 1 commit into from
Apr 1, 2025
Merged

edge_distances: fix test #106

merged 1 commit into from
Apr 1, 2025

Conversation

missinglink
Copy link
Contributor

@missinglink missinglink commented Aug 14, 2024

The fixture for this test case is confusing, it seems to 'want' false but actually it is being ignored and the test is expecting true.

Copy link

google-cla bot commented Aug 14, 2024

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.

@panmari
Copy link
Collaborator

panmari commented Apr 1, 2025

@missinglink for us to merge this PR, we need a signed CLA. Could you please check out the failing check above? Thanks!

@missinglink
Copy link
Contributor Author

Screenshot 2025-04-01 at 14 05 38 Screenshot 2025-04-01 at 14 05 42

@jmr
Copy link
Collaborator

jmr commented Apr 1, 2025

You have a CLA signed under peter@i-wont-dox-you.some.tld. You either need to sign another with the email address used for the commit or rewrite the commit to use the other email address.

The fixture for this test case is confusing, it seems to 'want' `false` but actually it is being ignored and the test is assertion for `true`.
@missinglink
Copy link
Contributor Author

Screenshot 2025-04-01 at 20 06 47

@@ -202,26 +202,26 @@ func TestEdgeDistancesUpdateMinInteriorDistanceRejectionTestIsConservative(t *te
a: Point{r3.Vector{1, -8.9031850507928352e-11, 0}},
b: Point{r3.Vector{-0.99999999999996347, 2.7030110029169596e-07, 1.555092348806121e-99}},
minDist: minDist,
want: false,
want: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed true agrees with C++.

https://github.com/google/s2geometry/blob/58de4ea1e2f8a294e0c072c602c22232fd1433ad/src/s2/s2edge_distances_test.cc#L204

@rsned Maybe we should just delete the want field, since it's always true.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably come up with At least one test case that should be false just to be sure things actually do as they claim.

@jmr jmr merged commit 35ab489 into golang:master Apr 1, 2025
2 checks passed
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.

4 participants