Refactored error checking tests. Fixes #71 #72
Conversation
|
Yikes, looks like the OSX build is a bit sick. Cannot start for the whole ten minutes, can't be a good sign. |
|
Give it time https://www.traviscistatus.com/ |
diffmatchpatch/diff_test.go
Outdated
| @@ -998,8 +998,38 @@ func TestDiffText(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestDiffDelta(t *testing.T) { | |||
|
|
|||
diffmatchpatch/diff_test.go
Outdated
| @@ -998,8 +998,38 @@ func TestDiffText(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestDiffDelta(t *testing.T) { | |||
|
|
|||
| type TestCase struct { | |||
| Text string | |||
There was a problem hiding this comment.
Please add a "Name" field too and use the comments for the test case values. You can pretty much copy the pattern (e.g. for the error messages for the asserts) for that from other test functions.
diffmatchpatch/diff_test.go
Outdated
| type TestCase struct { | ||
| Text string | ||
| Delta string | ||
| ErrorMessagePrefix string |
There was a problem hiding this comment.
Since ErrorMessagePrefix is part of the expected members please add a new line before the field. That is just the style of the other test functions.
diffmatchpatch/diff_test.go
Outdated
| {"", "--1", "Negative number in DiffFromDelta: -1"}, | ||
| {"", "", ""}, | ||
| } { | ||
| _, err := dmp.DiffFromDelta(tc.Text, tc.Delta) |
There was a problem hiding this comment.
Please test the first return argument too, it should be always nil because all cases have an error.
diffmatchpatch/diff_test.go
Outdated
| {"", "a", "Invalid diff operation in DiffFromDelta: a"}, | ||
| {"", "-", "strconv.ParseInt: parsing \"\": invalid syntax"}, | ||
| {"", "--1", "Negative number in DiffFromDelta: -1"}, | ||
| {"", "", ""}, |
There was a problem hiding this comment.
Please remove the empty case from the list, since it is not an error case. Just put it back below. You can then also remove the condition for the empty ErrorMessagePrefix below
There was a problem hiding this comment.
While I agree with the rest of your comments, I'm struggling with this one. Initially I went without the empty case, but then I saw that it fits the pattern, and a similar case is handled similarly in patch_test.go so I decided to move it in. I think the idea here is that checking for absence of error is also error checking, the other thing is the refactoring to avoid duplication.
There was a problem hiding this comment.
I agree on what you wrote. The absence of an error must be checked, of course. However, the linked test function handles both error and non-error cases, and I just went with your comment "Error checking" which suggested you wanted to verify only negative cases. As I mentioned in my other commit, it seems that I forgot to refactor this function with the table-driven pattern. I would therefore suggest to drop the "Error checking" comment, and try to put the other cases into the table-driven pattern too. What do you think?
|
I've looked at refactoring the other cases, but they all seem to have significant amounts of input data which is initialised differently on a case-by-case basis. I felt that refactoring these into the table pattern could make the actual table look very bulky and unpleasant. Please advise. |
|
Yeah, I did not find a good solution either. Have to ponder a bit more but let's merge your PR. It looks really good and its a great improvement! Just one small thing: Please squash the two commits |
|
I'm probably dumb, but I think I've now ended up with four commits instead of two as previously, but at least the changes are now combined into a single commit 1e4b8b1. I don't really know how to go around this. |
|
@zimmski do you want to merge this? or is there anything else i can do to improve it? |
|
@zimmski Ping! |
|
Pong! |
|
This PR still looks good to me. The only thing that is left is squashing the commits together. Take a look at this http://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git |
|
Basically you have to do the following:
I could do that myself and upload a new version of the PR with the commit in your name, but I think if you not too familiar with using Git's rebase functionality this would be a good training example. So if you want to let me do it, just tell me, I am also happy to help you out with the rebase. No problem :-) |
Code-review changes, update to an incorrectly worded error message
|
Finally, all done! Travis also picked up a bug occurring with 1.6.4 which I fixed as well. |
|
Ping @zimmski! |
|
I wished you had put that fix in another commit, but I'll take a look at the change and then merge. Sorry for taking ages. Extremely busy :-( |
|
I am running with Go 1.6.4, I cannot reproduce the problem of https://travis-ci.org/sergi/go-diff/jobs/218036777 |
|
However, the change looks good to me. |
No description provided.