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

Fix infinite loop bug #9

Closed
wants to merge 14 commits into from
Closed

Fix infinite loop bug #9

wants to merge 14 commits into from

Conversation

ctessum
Copy link

@ctessum ctessum commented Feb 12, 2016

The bug is defined in TestBug4. I also changed the package name in bugs_test.go so the tests would work in forked repositories.

@dmitshur
Copy link

I also changed the package name in bugs_test.go so the tests would work in forked repositories.

I personally don't think that's a good thing to optimize for. Instead, it's better to optimize for having better tests. If someone chooses to make a longstanding fork, fixing their tests will be a minor effort compared to maintaining and developing the fork.

In general, external tests should be slightly better since they (are guaranteed to) test the external API, which is what users of the package use.

Of course, the change is minor, but I just wanted to share that thought.

@ctessum
Copy link
Author

ctessum commented Feb 13, 2016

However, if one has a longstanding fork which they are regularly creating pull requests from, it could be a headache. Let me know what the consensus is.

@dmitshur
Copy link

Fair enough, in that case it's probably worth it.

@akavel
Copy link
Owner

akavel commented Feb 13, 2016

Uh, oh; I'm somewhat confused now. Given my comment in #4, I believed TestBug4 is already passing OK, no infinite loop and no need to fix - at the cost of re-introducing #3. Is that not so? Or does this PR fix #4 on older repo, where #3 was "fixed" but #4 was introduced? but your fork seems to be quite new, so shouldn't have #4 outstanding, "only" #3... could you please clarify? [I know this situation is somewhat messy in itself even without this PR, sorry. So let's try to work towards some mutual understanding.]

edit: please see e.g. the latest build on Travis-CI [if I dug it correctly] - it seems to show TestBug4 as passing, but TestBug3 failing. Or do both TestBug4 and TestBug3 fail on some computer of yours?

@ctessum
Copy link
Author

ctessum commented Feb 13, 2016

Sorry for the confusion. This is a new bug, apparently caused by floating point rounding error, apparently fixed by adding a tolerance to PointEquals.

I just added some potentially difficult test cases to TestBugs4, thinking that test could be used as a general repository for infinite loop bugs. Only one of them causes a problem with the current implementation, but all of the other ones started causing problems when I tried other fixes. With the fix I added they all appear to be okay.

So to clarify, this PR both adds a bug-identifying test and (apparently) fixes the corresponding bug. Let me know if you would like it organized differently.

@ctessum ctessum mentioned this pull request May 4, 2016
@ctessum
Copy link
Author

ctessum commented May 19, 2016

I'm going to close this because I accidentally made it against my master branch and now I want to make other changes to my master branch. I can re-open it with the specific changes to fix this bug if there is interest.

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.

3 participants