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

Consider adding a more comprehensive set of package-level assertions #49

Closed
alecthomas opened this Issue Feb 1, 2018 · 15 comments

Comments

Projects
None yet
2 participants
@alecthomas
Copy link

alecthomas commented Feb 1, 2018

Similar to testify/assert:

  • NotNil()
  • Nil()
  • Empty()
  • NotEmpty()

etc.

@alecthomas alecthomas changed the title Comprehensive set of package-level assertions Consider adding a more comprehensive set of package-level assertions Feb 1, 2018

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Feb 1, 2018

The design of the current API is something like this:

Top level assertions

The most common assertions (80% of calls) have a top-level assertion in the assert package which uses FailNow().

Equal and DeepEqual seem like obvious choices here. Many other assertions are handled by Assert() and Check() directly, so they don't need their own function:

  • NotNil is assert.Assert(t, x != nil) - on a failure this already prints all the information you need. The failure message would be assertion failure: x != nil is false. We know the value of x must be nil in this case.
  • NotEmpty is assert.Assert(t, len(x) != 0) - same as the above case, on failure we know the value of x
  • NotEqual is assert.Assert(t, x != 10) - same case, if this fails we know the value of x to be 10
  • Error (with error types) -> assert.Assert(t, os.IsNotExist(err), "got %+v", err)
  • True/False is assert.Assert(t, x), assert.Assert(t, !x)

(as a side node, I think I will probably look at rewriting the failure message to replace != with == and remove the "is false". This can be done without changing the API)

Currently assert.NilError exists as a top level assertion but I think it should be removed with e10aac4. assert.Assert() can handle error directly. With this change assert.NilError(t, err) becomes assert.Assert(t, err) and the failure message is the exact same.

DeepEqual is not yet a top level assertions, but I think it probably should be, it is very common.

Comparisons

For the next common case (the next 15% most common calls), and for any "assertion" that should use Fail instead of FailNow there are comparisons provided in assert/cmp.

Contains, Error, ErrorContains, Len, Nil, Panics. More can be added here.

The rest

For anything else (the last 5%) a local comparison can be written to compare values

As a result of that design most projects should be primary calls to a top level assertion, with a small number of calls that use assert.Check() or assert.Assert(t, someComparison).


That said, do you think there are other assertions that are common enough to be in the 80% use case?

Nil seems like it would be rarely used (excluding errors, which are already handled by Assert).
Empty I omitted because it seems like it's already handled by Len(x, 0), or "false". Is there some other case that isn't handled?
Len and Contains are weaker assertions than Equal (or DeepEqual), so by keeping them in the second category I think it helps encourage stricter testing practices.

Checking error values is potentially a lot more common, but there are so many different ways to do that. Error/ErrorContains are just fallbacks if there are no types, but many errors should either have their own type defined by an interface, or an IsErrOfKind() lookup. Both of those cases can be handled by the default assert.Assert().

If there are comparisons that do end up being more common we should definitely add them as top level assertions. If there are comparisons that aren't provided by cmp that are frequently used by multiple projects we should also get them included in cmp.

@alecthomas

This comment has been minimized.

Copy link
Author

alecthomas commented Feb 3, 2018

What's the rationale for not including helper functions? I guess I don't see the point in forcing users to import cmp unless they have very custom requirements.

Also, regarding the list of assertion expressions, I would suggest that assert.Assert(t, x != nil) is less obvious/explicit than assert.NotNil(t, x). Additionally the message x != nil is false is more confusing than should not be nil.

Here are the top ~20 stretchr/assert helper functions across all github repos in my $GOPATH:

5841 Equal
2049 NoError
 579 Nil
 542 True
 486 NotNil
 347 Error
 282 Len
 279 EqualValues
 278 False
 249 Contains
 139 Empty
 136 Panics
  96 EqualError
  61 NotEmpty
  56 NotEqual
  53 New
  36 IsType
  31 NotContains
  27 NotPanics
  23 Fail
  19 Regexp
@alecthomas

This comment has been minimized.

Copy link
Author

alecthomas commented Feb 3, 2018

Also I would add that if the API were a drop-in search/replace for stretchr/assert (s,stretchr/testify/(assert|require),gotestyourself/gotestyourself/assert,) it would make it much easier to adopt.

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Feb 4, 2018

What's the rationale for not including helper functions?

A couple reasons. One is that golang provides two ways to fail a test (Fail, FailNow). I think FailNow is the more common case, but Fail still has it's uses. The two main use cases I see for Fail are:

  • any function which returns multiple non-error values
  • any non-pure function where you want to check modified state as well as return value

In these cases it can be beneficial to use Fail (assert.Check) so that even if the first check fails you get more context about the failure by continuing to perform the other checks.

The convenience function can only use one of the failure functions (FailNow) or there would have to be duplicate functions to handle both. I'd like to avoid the duplication, so there will always be a need to use some comparisons from cmp.

A second reason is that I'd like the keep the assert package interface small and focused. I think it helps highlight the stricter (and more common) assertions. That said, right now at only 4 there is plenty of room to add more.

Finally, it's easier to add something than to remove it. Using go-cmp instead reflect.DeepEqual makes it easier to compare things with cmp.DeepEqual. I'm hoping that will make Contains and Len less common. So I'm hoping for the first release or two to keep the top level package fairly small, and add more as the library is used in a few repos to see which assertions could be promoted to the top level package.

I guess I don't see the point in forcing users to import cmp unless they have very custom requirements.

I hope that helps explain the rational. I also hope that the extra import isn't a significant barrier. In many cases testify required two imports as well (assert and require). Most IDEs should make it trivial to import things with a shortcut.

Thanks for asking these questions. I've been meaning to write down the rational for this design, and these questions help me organize my arguments.

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Feb 4, 2018

I would suggest that assert.Assert(t, x != nil) is less obvious/explicit than assert.NotNil(t, x).

Less obvious, I think I agree, but maybe that's because we're used to testify? A big influence for this library is pytest which did a lot of things differently from the traditional junit style assertions/framework. In pytest you write all assertions as assert x == y, assert x is None, etc.

I think it will become more intuitive, and if not it's never to late to add it.

I'm not sure if I agree about explicit. x != nil is a go expression that everyone should understand immediately. NotNil could contain hidden implementation details that aren't obvious.

Additionally the message x != nil is false is more confusing than should not be nil.

I completely agree, this is something I need to fix. I opened #50 to track it. I think it should be easy to translate != to is so that it will read assertion failed: x is nil

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Feb 4, 2018

Here are the top ~20 stretchr/assert helper functions across all github repos in my $GOPATH:

This is awesome, thanks for these numbers! I'd like to get these numbers for a few more go projects to see how they compare. If it turns out there are other common assertions then I'm definitely happy to add them as convenience functions.

Looking at your numbers, 80% would be the top 5 assertions, which are potentially entirely covered already by Assert/Equal (but will probably require DeepEqual) as well. I would need to look into why Nil is being used, if it's being used for errors, or other values. Same with EqualValues (why not just Equal?).

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Feb 4, 2018

Also I would add that if the API were a drop-in search/replace for stretchr/assert (s,stretchr/testify/(assert|require),gotestyourself/gotestyourself/assert,) it would make it much easier to adopt.

Ease of adoption is one of my main concerns. However, if we were to just copy the interface I think there would be very little reason to use gotestyourself/assert. Many of the major problems with testify are with the interface, not the implementation. So copying the interface means we aren't actually gaining much and there's no reason to migrate.

I would rather address this problem with an automated migration tool, see #36. It parses AST and replaces testify calls with new AST for gotestyourself/assert. I've used it on 5 fairly large go projects and it's been working well. It will print a message for any assertion that it can't automatically migrate so the user knows how effective it will by doing a dry run. It even handles assert.New().

There's more polish that could be added, but that can be added over time.

@alecthomas

This comment has been minimized.

Copy link
Author

alecthomas commented Feb 4, 2018

This is awesome, thanks for these numbers! I'd like to get these numbers for a few more go projects to see how they compare. If it turns out there are other common assertions then I'm definitely happy to add them as convenience functions.

Just to clarify: this isn't just my projects, this is all projects I have ever downloaded transitively or otherwise with go get.

Ease of adoption is one of my main concerns. However, if we were to just copy the interface I think there would be very little reason to use gotestyourself/assert. Many of the major problems with testify are with the interface, not the implementation. So copying the interface means we aren't actually gaining much and there's no reason to migrate.

What are your problems with the public interface for testify?

My two biggest issues with testify are that diffs often don't show the actual differing entries, just pointer addresses (which is completely useless), and that testify/assert doesn't fail the test by default. Both of these are what motivated my fork). Another issue is that the internal API isn't very flexible, which I think your project fixes.

Anyway, it sounds like you're happy to keep your current API and are trying to attract users who don't really like the testify API. Personally, I like it, so I'll stick with it.

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Feb 4, 2018

The issues I see with the interface are: test-go/testify#13

  • everything is duplicated as Foo and Foof, and essentially duplicated again in require
    • HTTP and JSON assertions belong in a separate package for testing http requests
    • lots of confusing overlap (Equal/Exactly/EqualValues, Zero/Nil/Empty/Len(x, 0), InDelta/InEpsilon)
    • unnecessary assertions (Fail, FailNow, True, and False provide nothing over the standard testing.T, NotPanics is effectively a no-op)
  • inconsistent parameter placement (Contains/Len/EqualError all accept the expected value as the second arg, but most other assertions accept the expected as the first)

Also not in that list:

  • too many weak assertions (NotEqual, NotZero, NotContains, NotRegexp) that should almost never be used because they aren't strict enough about expected values. In the very rare cases that are needed they can be package local comparisons.
  • FileExists/DirExists should be in a package for testing file properties/contents

Once all those API changes are made I think the end result will be what ends up in gotestyourself/assert.

I fully expect to add more package level convenience functions, but I'd rather wait to see what actually gets used frequently with the new API.

@alecthomas

This comment has been minimized.

Copy link
Author

alecthomas commented Feb 4, 2018

A big influence for this library is pytest which did a lot of things differently from the traditional junit style assertions/framework. In pytest you write all assertions as assert x == y, assert x is None, etc.

I've used pytest extensively and it is excellent... I was about to point out how pytest differs in that it reflects the assertion expression, and that made me realise that gotestyourself does the same thing...

However, I had absolutely no idea that gotestyourself did this, as it isn't mentioned anywhere in the README or godocs (that I could find)... this seems like the key differentiator between gotestyourself and testify, why isn't it mentioned anywhere?

The rest of this conversation now actually makes sense to me.

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Feb 4, 2018

Ah! Good call. I guess it's not documented anywhere. I will fix that soon.

@alecthomas

This comment has been minimized.

Copy link
Author

alecthomas commented Feb 4, 2018

Hahaha :)

@alecthomas

This comment has been minimized.

Copy link
Author

alecthomas commented Feb 4, 2018

I'll have a think and play around with it a bit, but I suspect I will not hold the position in this issue any longer and will agree with you 100%.

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented Feb 7, 2018

First attempt at documenting this in #57

@dnephin

This comment has been minimized.

Copy link
Member

dnephin commented May 2, 2018

Full list of the current asserts: https://godoc.org/github.com/gotestyourself/gotestyourself/assert#pkg-index
With some examples of how to do NotX: https://godoc.org/github.com/gotestyourself/gotestyourself/assert#hdr-Example_usage

DeepEqual is capable of excluding fields and customizing comparison so I think it takes care of most of the common cases.

Going to close this issue, but if there are specific cases that are commonly used and aren't covered I'd still like to hear about it.

@dnephin dnephin closed this May 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.