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

Bring coverage up to 100% line coverage #55

Closed
zimmski opened this issue Dec 2, 2016 · 7 comments
Closed

Bring coverage up to 100% line coverage #55

zimmski opened this issue Dec 2, 2016 · 7 comments

Comments

@zimmski
Copy link
Collaborator

zimmski commented Dec 2, 2016

There are some lines that still do not have test cases https://coveralls.io/github/sergi/go-diff It would be nice to bring the coverage up to a real 100% and then stay there by making decreasing coverage a PR failure.

@zimmski zimmski changed the title Bring coverage to 100% Bring coverage up to 100% Dec 2, 2016
@maksimov
Copy link
Contributor

maksimov commented Dec 3, 2016

I'm learning Go and I thought the best way to do it would be to improve some tests. I brought coverage up a notch from 99.3% to 99.4%, but then I started experimenting with a test for patch.go:546 and I've added this test-case (never mind the expected value, I was going to correct it):
{"@@ -0,0 +0,0 @@\n+abc\n", "Bad patch mode"}

Now my test fails like this:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x1398840]

goroutine 51 [running]:
testing.tRunner.func1(0xc4201c40e0)
	/usr/local/go/src/testing/testing.go:644 +0x55d
panic(0x13f3de0, 0x1620fb0)
	/usr/local/go/src/runtime/panic.go:489 +0x2ee
github.com/maksimov/go-diff/diffmatchpatch.TestPatchFromText(0xc4201c40e0)
	/Users/stas/Fun/TryGo/src/github.com/maksimov/go-diff/diffmatchpatch/patch_test.go:81 +0xb90
testing.tRunner(0xc4201c40e0, 0x145ace8)
	/usr/local/go/src/testing/testing.go:679 +0x229
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:716 +0x53d

Do you think it's a genuine candidate for code improvement or am I being silly with my test case?
Thanks!

@maksimov
Copy link
Contributor

maksimov commented Dec 3, 2016

Yes, I guess it's my test case. The panic is in the test code.

@zimmski
Copy link
Collaborator Author

zimmski commented Dec 3, 2016

I did not try it but when I guess that the test panics because "err" is nil so the "Error" call fails because of that. I think it is NEVER silly to improve a project ;-) To be honest, if you are just learning Go it would be more fun to write new code than adding test cases. Finding test cases for this project is especially hard, there aren't many instances left and the easy ones are already gone.

@zimmski
Copy link
Collaborator Author

zimmski commented Dec 3, 2016

Maybe #54 is more fun since you would write new code, must test it, and then have to benchmark your solution. The marker is here

// TODO research and benchmark this, why is it not activated? https://github.com/sergi/go-diff/issues/54
But beware that if the solution has bad benchmark results, we cannot merge it.

@zimmski
Copy link
Collaborator Author

zimmski commented Dec 3, 2016

I looked through the open issues and I think #49 could be also mildly interesting since the whole project is missing example for every exported API endpoint. #57 and #58 could be also interesting, at least there is new code involved. However, if you are just learning Go I am a little lost on what might be the most interesting and easiest issue for this project.

@maksimov
Copy link
Contributor

maksimov commented Dec 3, 2016

Thanks very much for your help, @zimmski! I'll try a few things and see how I get on.

@zimmski zimmski changed the title Bring coverage up to 100% Bring coverage up to 100% line coverage Dec 5, 2016
@zimmski
Copy link
Collaborator Author

zimmski commented Apr 9, 2017

@maksimov I think with your changes the bar has been raised high enough to close this issue

@zimmski zimmski closed this as completed Apr 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants