go-test / deep

Golang deep variable equality test that returns human-readable differences
MIT License
743 stars 54 forks source link

Add option that causes nil slices to equal empty slices. #27

Closed Anaminus closed 4 years ago

Anaminus commented 5 years ago

Often times when testing, the difference between a slice that is nil and a slice that contains zero elements isn't useful. This change adds the NilSlicesAreEmpty variable, which is initially false to retain backward compatibility. When set to true, slices are compared by length rather than by nilness. Care has been taken to ensure that the resulting diff still indicates that a slice is nil versus empty.

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 254cd479303a51235cd3e2b9313457b72b87f5a6 on Anaminus:nil-slices-are-empty into 042da051cf3189d80f667339b822c7effac29cd6 on go-test:master.

daniel-nichter commented 4 years ago

I'm ok with the option, but can you refactor the logic a little to make it easier to follow? I think in original code, the block on line 276 that starts with if a.IsNil() || b.IsNil() {should be kept and this new logic put in there. Reason being: that block is all the logic for one or the other slice being nil. And the return on 282 implicitly means "both are nil" (there should be a code comment for that).

For tests, TestNilSlicesAreEmpty is great, thank you. Let's also make a new test case for

a = a[:1]
b = b[:0]

because TestSlice() is getting too big and because I didn't code comment it enough, it's not clear any longer what all it's testing.

Anaminus commented 4 years ago

Note: In a stunning turn of events, git was used poorly.

The additional logic cannot be contained within the block as you suggest. As expressed in the PR, the primary branching point must occur at NilSlicesAreEmpty.

When NilSlicesAreEmpty is true, the code must compare emptiness instead of nil-ness. The expression a.IsNil() || b.IsNil() blocks the case where either slice is non-nil but empty, so it is insufficient for comparing emptiness. This check is only useful after determining that NilSlicesAreEmpty is false.

I think the only revision that needs to be made here is to move the aLen and bLen declarations back to their original positions. Caching the lengths a little earlier doesn't really save anything, and introduces branches where they are declared, but unused.

As for the tests, I've moved the additional cases in TestSlice to a separate TestEmptySlice function. All additional cases have also been commented to make them easier to distinguish.

ktnyt commented 4 years ago

Hello, has there been any progress on this PR? I've just started using this library and stumbled across a case where I would like an empty slice to equal nil. I can just implement an Equal method but I do believe some people would benefit from having this feature.

daniel-nichter commented 4 years ago

Yeah, just need to fix the merge conflict and double check the tests. Will do that and cut a new release soon.

daniel-nichter commented 4 years ago

Released as v1.0.5