Closed justinfx closed 5 years ago
Firstly, thanks very much for your feedback; so glad you like it!
About DeepEqual, yes, the cmp package deliberately refuses to compare unexported fields by default. This is actually a very good thing, as it stops you relying on internal details other packages that might change in the future and break your tests.
You can allow comparison of unexported fields by using cmp.AllowUnexported.
For example: https://play.golang.org/p/VPEQD4DemRc
If I'm doing this for a given set of structs quite a bit, then it can feel verbose, so I tend to move the comparer into a global variable. For example:
var deepEquals = qt.CmpEquals(cmp.AllowUnexported(myStruct{}))
Then I can do:
c.Assert(x, deepEquals, y)
This applies to all the other goodness supplied by the cmp package, of course (e.g. ignoring order in slices).
You're right that there's no other way to compare booleans. I'm not sure that it's worth adding two more checkers when c.Assert(x, qt.Equals, true)
is so clear and only 6 more characters to type.
You are free to create your own IsTrue and IsFalse checkers if you find this a burden, but then you'll have a little conundrum: should this code succeed or fail?
type myBool bool
c.Assert(myBool(false), IsFalse)
There are two arguably correct answers there, which makes IsTrue a less obvious addition than one might think.
Actually I was trying to do the opposite and get DeepEquals to ignore the unexported fields. This looks like it's working and matches my original use case:
https://play.golang.org/p/DteA5Wcj2hj
For the bool example, I guess I don't understand the ambiguity you are proposing.
https://play.golang.org/p/DWEBbszeseC
It does exactly what I would expect a truthy bool test to do. Just figured it would shorten the ability to test Booleans.
Thanks for the help!
Actually I was trying to do the opposite and get DeepEquals to ignore the unexported fields.
Ah, sorry. You said you were trying to match the usage of reflect.DeepEqual
which does not ignore unexported fields.
For the bool example, I guess I don't understand the ambiguity you are proposing.
The problem is that when you pass a constant to a function, as we're doing with Assert, it's not an untyped constant any more:
Oh good point about DeepEquals. I was focused more on transparently swapping about one for the other and it keep working, as opposed to identifying correctly which comparisons it had been doing. All good since I understand how to modify the cmp function now.
For your bool example, I honestly feel it makes an even better case for having first class support for asserting truthy expressions. I wanted it to perform the equivalent of "if x" or "if !x" as opposed to an equality test between two values. Technically the way it is now, I have to be very explicit when checking truthy against custom types. I hit the same problem where a previous test was checking a uint32 against a literal number. This failed when I converted it to the assert and I had to specifically convert the expected literal to uint32(0)
.
In many languages the assert
statement accepts a boolean. Requiring a qualifier seems a bit strange.
Why not have the single argument form of Assert()
work that way?
For example https://godoc.org/gotest.tools/assert#Assert can accept either a comparison or a boolean. Which lets you do things like:
assert.Assert(t, ok)
assert.Assert(t, !missing)
assert.Assert(t, total != 10) // NotEqual
I'm not familiar enough with the failure messages of this library, but in the case of gotest.tools/assert, the literal AST is printed as the failure message, so you don't need extra qualifiers to create a message. You can say total is 10
or missing is true
, or ok is false
by translating the AST: https://github.com/gotestyourself/gotest.tools/blob/master/assert/assert.go#L156-L203
Why not have the single argument form of Assert() work that way?
I initially would have wanted to use it that way but this library doesn't have a single argument form of Assert() and I assumed the goal of the project was to have a small api surface, so another form of Assert would have been out.
In many languages the assert statement accepts a boolean. Requiring a qualifier seems a bit strange.
If you just accept a boolean, you can't print out the details of the assertion values that contributed to the assertion failure. For example, If I wrote:
c.Assert(v == 5)
how could quicktest print the actual value of v
in the test failure?
how could quicktest print the actual value of v in the test failure?
For that particular case it can't, which is why an Equal assertion is still important. However there are cases where it cane provide all the detail that you need:
x != nil
, or x != 10
), x can only be 1 possible value if the assertion failsFor both of those cases printing the AST gives you all the information you need. I believe point 2 in the issue description was about the boolean case specifically, not assertions in general.
For both of those cases printing the AST gives you all the information you need.
For the second example, that's only the case if the argument is a literal, rather than some other value that might vary at runtime.
For the first case, perhaps there's room for IsTrue
and IsFalse
, although personally, I've found them rare enough that just writing c.Assert(v, qt.Equals, true)
is explicit and not enough more verbose to be a pain to write out or read.
I think that https://github.com/frankban/quicktest/issues/52 (that I just filed) captures the last comment from Roger and the outcome of this discussion, so closing this.
Just started using this library today, and I really like it so far! It is a much tidier way to introduce some much needed asserts without a huge testing framework. I also love that it replaces some of the existing lifecycle management I had already been doing. A few questions about the usage so far...
1) I don't seem to understand how to customize the usage of
DeepEquals
/CmpEquals
to match the usage ofreflect.DeepEquals
. I had some code that was checking equality on the*exec.Cmd
type to ensure a command is generated the way it is expected. But when it runs throughquicktest.DeepEquals
, it fails because of unexported fields. I tried adding thecmpopt
dependency and passing different combinations of options but I couldn't get it to compare this type the same way as the stdlib. Is there any more detailed usage examples on this?2) Am I overlooking a less verbose way to assert a
bool
value than doingc.Assert(val, qt.Equals, false)
? Are there thoughts on havingc.Assert(val, qt.True)
andc.Assert(val, qt.False)
?