Fwd: New checker "jc.IsNil"

[roger peppe]

On 24 November 2014 at 02:39, Tim Penhey <tim.penhey at canonical.com> wrote:
> On 24/11/14 14:02, Tim Penhey wrote:
>> On 19/11/14 01:16, roger peppe wrote:
>>> On 18 November 2014 04:22, Tim Penhey <tim.penhey at canonical.com> wrote:
>>>> 'ello 'ackers,
>>>>
>>>> This is something that I have been meaning to add for quite some time.
>>>>
>>>> We have been recording stack traces with the juju/errors library, but
>>>> until now, they have been of limited use.
>>>>
>>>> One of the key places I have wanted to see the stack trace for a while
>>>> is when running tests, and I have a function that returns an error, I'd
>>>> normally have:
>>>>
>>>>   c.Assert(err, gc.IsNil)
>>>>
>>>> to make sure the error is nil.  Now I have added a checker into the
>>>> juju/testing/checkers that is error aware.
>>>>
>>>> If you now use jc.IsNil instead of gc.IsNil, and the value is an error,
>>>> and the error has a stack trace, it will get printed out.
>>>>
>>>> Very handy.
>>>>
>>>> The dependencies of master have the versions you need to use it now.
>>>>
>>>> Cheers,
>>>> Tim
>>>
>>> One thing that I've been meaning to do for a while is
>>> change all occurrences of c.Assert(err, gc.IsNil) to
>>> c.Assert(err, gc.Equals, nil), because technically the former is
>>> wrong, as it could be a typed nil but still erroneously
>>> pass the test.
>>
>> Interesting.  I've modified my checker to reject typed nils, and am
>> currently running it over the entire codebase. A few intermittent
>> failures the first time through, and a place I missed. Currently running
>> the second time through.
>
> Actually I thought it was going this, but when I wrote a test to
> confirm, I found that there isn't really a way to check in the checker
> the difference between a typed-nil and a nil pointer to a type..
>
> Since the value is passed into the checker as an "interface{}", if it
> was a *foo and nil, the interface records those types.  If it was a
> typed nil in an interface, it ends up looking like a *foo that is nil.
>
> IFAICT we have four options:
>
>  * gc.Assert(err, gc.Equals, nil)
>  * write an explicit nil checker that *ONLY* passes for '== nil'
>  * write an explicit error nil checker
>  * do nothing, continue using jc.IsNil and code reviews
>
> I'm probably, -1, -1, +0, and +1 respectively on the above options.
>
> Curious about others.

Of those four, the first one exactly expresses the assertion we actually
want to make, and is logically deducible from the existing gocheck
primitives, but I presume you don't like that because it doesn't
make it possible for you to hook in your error printing code
without implementing GoString.

Given that, then I'd suggest that the third option is preferable,
("ErrorIsNil" perhaps, to go along with ErrorMatches), as
it makes it clear from the name why this checker is different
from other checkers (as jc.IsNil does not) and it gives additional
freedom to check that a typed nil does in fact implement the
error interface.