Document guidelines for reviewing proposed additions to the API for v2.0.0

[brackendawson]

Testify frequently sees requests for new assertions, and we are trying to make the API more consistent in v2.0.0. It's not always clear what should be included (as evidenced by some of the more clunky functions already in the API). I propose that we should write as a wiki page or maybe a markdown file somewhere; the agreed standards which an assertion should meet before it is included in the API.

I've written a draft. It is only my own opinions so please do critique it rigorously. I expect there are good assertions in the API now that contravene some of these ideas.

Proposed Assertion Guidelines

This document is a set of guidelines for the inclusion of the existing and any new assertion functions in the v2.0.0 release.

1. Language
2. Argument Order
3. Argument Names
4. Meta Assertions
5. Inverse Assertions

Language

The function name and argument list should lead to use which documents the intended assertion in reasonably good but terse English.

Good ✅

assert.Equal(t, got, 5)
assert.Between(t, got, 2, 10)

Unclear ❌

assert.InDelta(t, got, 6, 4)

Argument order

As #399 states, the order of arguments should be:

1. TestingT
2. The actual result being tested if distinct from the expectation, or the logical plain English order.
3. The expectation being asserted, or the plain English order.
4. msgAndArgs ...interface{}

Eg

Equal(t TestingT, actual, expected interface{}, msgAndArgs ...interface{}) bool
Contains(t TestingT, s, contains interface{}, msgAndArgs ...interface{}) bool
Less(t TestingT, e1, e2 interface{}, msgAndArgs ...interface{}) bool

Format functions (Assertionf(t TestingT, actual, expected, msg string, args ...interface{})) should not be accepted as they are equivalent to their "non-f" counterparts.

Argument Names

If there is a likely actual value and expected value; then actual and expected should be the argument names.

Equal(t TestingT, actual, expected interface{}, msgAndArgs ...interface{}) bool

If there is no obvious actual value and there are multiple values of the same type; then consistently use v1, v2, ... to differentiate the vs from any other arguments that might be present.

Less(t TestingT, v1, v2 interface{}, msgAndArgs ...interface{}) bool

If there is no obvious actual value and there is only one value, or values of different types; then take example from the standard library or use plain english. Be careful not to follow the stdlib examples if their argument type is descriptive but the assertion uses interface{}.

// error is frequently called err
NoError(t TestingT, err error, msgAndArgs ...interface{}) bool
// sort.SearchInt(a []int, x int) uses a for the slice and x for the element, but the types are critical so this example is bad.
// strings.Contains(s, substr string) is a better example, but s is short for string.
// Just use English or well known abbreviated English for both arguments.
Contains(t, container, val interface{}, msgAndArgs ...interface{}) bool

Meta Assertions

A wrapper around an existing assertion should exist only if it improves on the readability of a pure Go implementation.

Useful ✅

assert.ElementsMatch(t, got, expected)visited := make([]bool, len(got))

// - vs -

visited := make([]bool, len(got))
for i := 0; i < len(expected); i++ {
	found := false
	for j := 0; j < len(got); j++ {
		if visited[j] {
			continue
		}
		if got[j] == expected[i] {
			visited[j] = true
			found = true
			break
		}
	}
	if !found {
		t.Error("Elements do not match")
	}
}
for j := 0; j < len(got); j++ {
	if visited[j] {
		continue
	}
	t.Error("Elements do not match")
}

Less valuable ❓

assert.InDeltaSlice(t, got, expected)

// - vs -

if assert.Equal(t, len(got), len(expected)) {
	for i := range got {
		assert.InDelta(t, got[i], expected[i], 5)
	}
}

Inverse Assertions

I don't know whether to do anything here, it's tempting to say that all inverted functions should be prefixed with "Not", eg: Equal() and NotEqual, But NoError() is perhaps better Language than NotError() and Less() is definitely better than NotGreater().

IsNonDecreasing() however is terrible, every assertion could be prefixed with "Is".

[Antonboom]

Format functions (Assertionf(t TestingT, actual, expected, msg string, args ...interface{})) should not be accepted as they are equivalent to their "non-f" counterparts.

I think it would be better to just make a cleaner and stricter API:

Equal(t TestingT, actual, expected any) bool       // No msgAndArgs!
Equalf(t TestingT, actual, expected any, msg string, args ...interface{}) bool

like

package fmt

Print(a ...any) (n int, err error)
Printf(format string, a ...any) (n int, err error)

And go vet will be happy 🙂

[dolmen]

A maintainer's manifesto about Testify v2

A v2 would please users of v2 because of a cleaner API.
A v2 would please maintainers of v2 who are fed-up with flaws in v1's API (traps).

But v2 will be of no help for users of v1 because that will require migration (even just adding /v2 to every import would be a significant and painful migration). With migration (changing code that works) comes a risk of regression. Testify is a set of frameworks for writing testsuites. A testsuite is one critical piece of a software project to protect against regression. The last thing someone wants is a regression in the testsuite that is supposed to protect against regressions.

So users of v1 who can't afford migration (rewrite cost, regression risk...) will just stay with v1. If v1 is abandoned by Testify maintainers who moved to v2, v1 users will just be orphaned.

Testify is very old and has a ton of users that rely on it. At the time of this post (2023-11-15) pkg.go.dev counts 12,454 public packages dependending on it. But this is only the tip of the iceberg as this doesn't count entreprise code hidden behind walls.

To not orphan users of v1, v1 would have to be maintained along with v2. So what is the benefit for maintainers if they have to maintain both v1 and v2? None: only more work while maintenance resources for v1 is already on shortage.

So, as the current only active maintainer, I'm declaring that v2 will never happen. Or at least a v2 of the github.com/stretchr/testify module with such major breaking changes.

Anyone is free to fork the project under a new module name, using changes proposed here or elsewhere, keeping the Testify spirit. This will let users migrate at their own pace. As a maintainer of Testify I will even gladly bless such a fork(s) by linking from testify's documentation (godoc, README). But there is no sane reason for this to happen with the github.com/stretchr/testify module name (the only reason would be branding, but I reject it as a sane reason). Anyway, with how Go modules work, a v2 would already be a module name change. So proponents of breaking changes must just recognize this and just use a fully separate module name.

In this repo we will just continue to maintain v1.
Forever.

Olivier Mengué (@dolmen), current only active maintainer of Testify.

PS: @Antonboom is doing an amazing work with testifylint. That is a major tool that helps Testify users to avoid v1's traps. More than a v2.