testify icon indicating copy to clipboard operation
testify copied to clipboard

Standardize function parameter order

Open zlotnika opened this issue 5 years ago • 9 comments

I have noticed that the parameter ordering seems inconsistent to what I would expect. An example of this:

resultJSON := map[string]interface{}{
  "status": "STATUS",
  "fields": map[string]interface{}{
    "id": "ID",
      },
    }
require.Equal(t, "STATUS", resultJSON["status"]) // expected actual
require.Contains(t, resultJSON["fields"], "id") // actual expected

Notice that in Equal, we give first the expected value, then the actual value whereas in Contains, we give first the actual map, then the expected value. I'd recommend always doing test struct, then the thing you're testing, then expected value (if it's a comparison).

zlotnika avatar Jul 25 '18 18:07 zlotnika

This sometimes trips me up a bit too. I prefer "expected actual" ordering.

But Contains isn't an expected-actual check, it's a containment check, that the "lhs contains the rhs", and in that sense, the ordering makes sense.

thwarted avatar Jul 26 '18 02:07 thwarted

This makes sense to think of these tests (Equal and Contains) as not totally equivalent. I do, however, think they fall under the category of having 1 thing to test (a string that might be "STATUS", a map that might have an "id"), and 1 naturally hard-coded value to test for (the string "STATUS", the string "id").

The reason I'd suggest "thing to test (actual)" first would be so that Nil, NotNil, Empty, etc look the same (just with a cut off argument). On the other hand, you could say the expectation defines the function being run on the actual (something like ExpectAn(t, expectation, actual) ~= ExpectAnExpectation(t, actual)), and so in that sense it makes more sense to have the expectations, if any, come first. I don't care strongly which comes first, but I think there could be consistency in that ideal.

zlotnika avatar Jul 26 '18 19:07 zlotnika

Now that Go 1.11 has modules and supports semver, I propose that the version is bumped to 2.0 and the API is cleaned up. It's long overdue.

Another option, because the current reliance on argument order is ambiguous, is to break up the API further into an "expect"-like API, e.g.:

// Before
assert.Equal(t, 42, answer)
assert.Contains(t, names, "bob")

// After
assert.Expect(t, answer).Equal(42)
assert.Expect(t, names).Equal("bob")

atombender avatar Aug 30 '18 19:08 atombender

I quite like the simplicity of the existing API in that there doesn't need to be the rspec-like Expect().MoreWords() syntax which is longer and more complex, but I agree the ordering of the parameters is confusing as they are today.

@thwarted Could you elaborate why you like the actual-expected ordering?

While I agree with your statements that there are specific differences between Equal and Contains that mean Contains should have the actual on the lhs, the fact that there are some functions like Contains where having the actual listed first is very specifically desirable raises the question why we don't have all functions have the actual listed first, since it is not as important which comes first for Equals.

leighmcculloch avatar Sep 22 '18 19:09 leighmcculloch

Also, we should be making it as easy as possible to transition to and from Go tests that do not use testify, so that testify is a least jarring experience. As a comparison to go tests that use only testing, it is very common for Go tests to list actual first, then expected. In the go stdlib and many libraries these are described as got and want, and got almost always comes first. There are many examples in the go stdlib if you search here:

https://github.com/golang/go/search?q=got+want

https://github.com/golang/go/blob/a9dcbab/src/syscall/js/js_test.go#L44-L48 https://github.com/golang/go/blob/50bd1c4/src/math/big/decimal_test.go#L24-L26

leighmcculloch avatar Sep 22 '18 19:09 leighmcculloch

@thwarted Could you elaborate why you like the actual-expected ordering?

I don't prefer actual-expected. In the second comment, I said «I prefer "expected actual" ordering.». To explain though, I think I prefer expected-actual order because actual is a more variable input, and I'm more comfortable with variable things at the end of argument lists (similar in idea to things like varargs always filling up the end -- obviously this isn't tantamount to "variable number of arguments", but that's some of the way I think about it). Things that are not changing, like literal values, come first, like the printf format string; and variable things and more complex (or longer) expressions come later.

assert.Equal(t, 42, 6 * 7)
assert.Equal(t, 6 * 7, 42)

I find the second one harder to read, because the more complex expression comes first. Imagine a series of these, they would be difficult to format align (if that was such a thing that gofmt didn't massage).

But while we're expressing preferences, I don't like the Expect().MoreWords() style because it reads too much like English. I realize this is probably a lame reason, I don't have a good rationale. Chaining methods isn't a common golang thing, in my experience, and doesn't lend itself well to error handling (not that this is necessarily an issue with using this form for test assertions, it's just more popular in other languages where the focus is often to explicitly "create a DSL". Does go need a DSL for tests? Would that be better?).

I like that this testify package makes writing tests easier and shorter. That would go away with more verbosity. I've got an in-house module that uses jsonpath to provide similar succinctness for doing JSON structure/value testing (we were using assert.Contains on strings for that, which isn't very robust), and I did pause for a bit when writing it to see if the argument ordering for that use case could be made "consistent" with other uses.

I'm probably going to have to continue to have the documentation open anyway. I don't consider this to be a hindrance. I think there is some inconsistency, but I also think my earlier observation (But Contains isn't an expected-actual check) stands. I think of this the same way some people think the ordering of arguments to memcpy/memmove are confusing because it doesn't match other APIs that order src-dest (aka from-to) ordering: the ordering of memcpy arguments match the ordering of arguments of the assignment operator (memcpy(dest, src, …) == dest = src). The memcpy argument ordering is consistent with what it is meant to support that C doesn't have directly (using the assignment operator on memory ranges).

thwarted avatar Sep 24 '18 05:09 thwarted

@thwarted I apologise, that was a typo on my part. I meant to ask, could you elaborate why you prefer expected-actual.

Either way, thanks for elaborating.


I like that this testify package makes writing tests easier and shorter.

💯 This is especially testify's strength. There are other packages like Ginkgo that offer support for rspec-like DSL Expect(...).To(Equal(...)) and for those packages that is their strength.


One reason I find actual-expected natural is it fits left-to-right in my brain with how I read expressions. I say to myself 6 x 7 should be 42. I write if statements the same way, if x == 42. I write sql the same way, where x = 42.


Another reason I find actual-expected natural is so many other testing libraries/frameworks I've used are actual-expected.

actual-expected

expected-actual

These are just ones I've come across. I'm going to guess there are more frameworks in both camps. I find it interesting that so many of the Go ones exhibit actual-expected, and I think there's value in being consistent with them.

leighmcculloch avatar Sep 24 '18 06:09 leighmcculloch

Perl's Test::More also uses actual-expected (aka got-want)

frioux avatar Dec 17 '19 00:12 frioux

I personally prefer the actual, expected ordering which makes more sense, because that's how we write if statements: number == 3 ("number equals three"), not 3 == number ("three equals number").

The latter just feels very counter-intuitive and hard to understand quickly in my opinion.

I've just been staring at a failed assert message wondering why it was expecting the wrong value and noticed that Equal()'s parameter order was the opposite of what I thought it was.

Was there a specific reason for this design choice?

NinoM4ster avatar Sep 28 '22 19:09 NinoM4ster