frankban / quicktest

Quick helpers for testing Go applications
MIT License
529 stars 27 forks source link

A little feedback #41

Closed akyoto closed 5 years ago

akyoto commented 5 years ago

Few notes from porting a testify/assert module to quicktest:

  1. quicktest introduces a boilerplate line where I have to create a New checker for each test. This is great for not having to add the parameter to each assert call, but it also makes me write a new line for each test that has no connection to what I'm actually testing. This is a design decision because it has pros and cons and I have mixed opinions on this - but leaning towards testify/assert having the better design in this case.
  2. I was missing a qt.Contains similar to assert.Contains. I don't like to rewrite every Contains call as a regex as they are harder to read.

While I don't expect any changes to 1) because it's probably something you already agreed on, I might be able to send a PR for 2) if this fits your vision of the module.

rogpeppe commented 5 years ago

Thanks for the feedback! As you suggest, the need for New isn't going to go away. It's the existence of the *qt.C type that makes possible the orthogonality of checkers with respect to Assert and Check, and hence a large part of the reason that quicktest can be small but extensible.

A Contains checker might well be a good addition though, although I'm a bit concerned that you might want to use different kinds of equality test (Equals, DeepEquals, CmpEquals). I have another idea that I think you'll like (somewhat more general). I'll make a PR soon.

akyoto commented 5 years ago

It is true that Contains can theoretically just be:

c.Assert(strings.Contains(a, "abc"), qt.Equals, true)

I just think this one looks cleaner from an end-user / reader perspective:

c.Assert(a, qt.Contains, "abc")

Aside from (string, string) bool this could potentially accept:

It is a bit "sugar-y" but then again that's the whole point of testing frameworks.

rogpeppe commented 5 years ago

The problem with c.Assert(strings.Contains(a, "abc"), qt.Equals, true) is that it doesn't print anything useful on failure. That's why we like to use Satisfies instead of just asserting a result boolean.

This is what I've done for Contains. I've already written the code - just a bunch of tests still to go.

// Any returns a Checker that uses the given checker to check elements
// of a map, slice or array. It succeeds if any element passes the
// check.
//
// For example:
//
//  c.Assert([]int{3,5,7,99}, qt.Any(qt.Equals), 7)
//  c.Assert([][]string{{"a", "b"}, {"c", "d"}, qt.Any(qt.DeepEquals), []string{"c", "d"})
//
// See also All and Contains.
func Any(c Checker) Checker

// All returns a Checker that uses the given checker to check elements
// of a map, slice or array. It succeeds if all elements pass the check.
// On failure it prints the error from the first index that failed.
//
// For example:
//
//  c.Assert([]int{3, 5, 8}, qt.All(qt.Not(qt.Equals)), 0)
//  c.Assert([][]string{{"a", "b"}, {"a", "b"}}, qt.All(qt.DeepEquals), []string{"c", "d"})
//
// See also All and Contains.
func All(c Checker) Checker

// Contains is a checker that checks that a map, slice, array
// or string contains a value. It's the same as using
// Any(Equals), except that it has a special case
// for strings - if the first argument is a string,
// the second argument must also be a string
// and strings.Contains will be used.
//
// For example:
//
//  c.Assert("hello world", qt.Contains, "world")
//  c.Assert([]int{3,5,7,99}, qt.Contains, 7)
var Contains Checker = ...
akyoto commented 5 years ago

This is brilliant and far better than what I initially suggested. I love the Any idea.

frankban commented 5 years ago

This is now implemented in v1.4.0.