Fwd: New checker "jc.IsNil"

[Tim Penhey]

On 24/11/14 21:05, roger peppe wrote:
> 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.

Partly, about GoString, partly about it not working everywhere.

To answer John's question about GoString, to me it just 'feels' wrong. I
like the way that the default GoString shows me the struct internals,
and I like having an easy way to show that. It is also about being
explicit about what you want, and for me, I trust my 'feel' when doing
more API design.

About it not working everywhere, we use gc.IsNil in many places.  The
IsNil checker works with channels, maps, slices, pointers and
interfaces.  Generally checking empty slices and maps is probably better
done with gc.Len, but not going into that.

The main issue with using `c.Assert(value, gc.Equals, nil)` is that if
you are looking at a pointer value, like *Anything, then this will fail
if the value is nil because the interface tuple (*Anything, nil) does
not equal the interface tuple of nil (nil, nil), and this is very
non-obvious for most test writers and readers.

> 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.

This is the conclusion that I came to as well.  An explicit NilError check.

Since typed nil values are more concerning when dealing with errors, we
should have a nice message if the interface supports error, has a nil
value but isn't `nil`.

I think I'll write and propose this.

Tim