Remove pkg/testutil/assert in favor of testify by aaronlehmann · Pull Request #32614 · moby/moby

aaronlehmann · 2017-04-13T23:41:54Z

I noticed that we're using a homegrown package for assertions. The
functions are extremely similar to testify, but with enough slight
differences to be confusing (for example, Equal takes its arguments in a
different order). We already vendor testify, and it's used in a few
places by tests.

I also found some problems with pkg/testutil/assert. For example, the
NotNil function seems to be broken. It checks the argument against
nil, which only works for an interface. If you pass in a nil map or
slice, the equality check will fail.

In the interest of avoiding NIH, I'm proposing replacing
pkg/testutil/assert with testify. The test code looks almost the same,
but we avoid the confusion of having two similar but slightly different
assertion packages, and having to maintain our own package instead of
using a commonly-used one.

In the process, I found a few places where the tests should halt if an
assertion fails, so I've made those cases (that I noticed) use require
instead of assert, and I've vendored the require package from
testify alongside the already-present assert package.

cc @dnephin @vdemeester

dnephin · 2017-04-13T23:59:53Z

SGTM

The functions are extremely similar to testify

By design! So that someday we could do exactly this. At the time pkg/testutil/assert was added we weren't using testify yet.

testify/assert also has an additional features that makes it easier to use with tabletests. The assertions accept an additional arg that you can pass a string which describes the test. It makes it a lot easier to find the failing case in a table of tests.

dnephin · 2017-04-14T00:03:03Z

I think the only thing missing from testify is a single line assert.Error() statement. The testify one only takes an error, so you have to write this:

require.Error(t, err)
assert.Contains(t, err.Error(), expectedError)

The problem is that I think this encourages people to skip checking the error message and only check that there is some error.

Maybe we could leave testutils/assert for a couple helpers and just change their implementation to use testify?

aaronlehmann · 2017-04-14T00:05:48Z

You can do require.Error(t, err, expectedError) with testify. I have this in many places in the PR.

The difference between testify and testutil/assert is that it checks for an exact match on the error message (which arguably is a good thing to encourage in tests). If you want to check for a substring, it does take an extra line.

I only encountered one place where this came up. If it turns out to be generally useful, I'm fine with having a helper function to do it in a single line (perhaps in testutil/helpers?)

dnephin · 2017-04-14T00:23:12Z

Ah, so there is require.EqualError(), I might try submitting a PR for require.ErrorContains()

aaronlehmann · 2017-04-14T00:29:20Z

Ah, yes, you need to use EqualError for that behavior.

Most of the places in this PR unintentionally dropped the error string checking. I'll either add the helper or update tests to check for full error strings where appropriate.

vieux · 2017-04-14T00:33:53Z

SGTM

aaronlehmann · 2017-04-14T01:32:30Z

Updated the PR to restore error substring checking. Will work on converting cli tomorrow.

vieux · 2017-04-14T01:41:47Z

@aaronlehmann at least on powerpc, tests are related

aaronlehmann · 2017-04-14T03:14:58Z

Missed a git add :). Will fix tomorrow.

I noticed that we're using a homegrown package for assertions. The functions are extremely similar to testify, but with enough slight differences to be confusing (for example, Equal takes its arguments in a different order). We already vendor testify, and it's used in a few places by tests. I also found some problems with pkg/testutil/assert. For example, the NotNil function seems to be broken. It checks the argument against "nil", which only works for an interface. If you pass in a nil map or slice, the equality check will fail. In the interest of avoiding NIH, I'm proposing replacing pkg/testutil/assert with testify. The test code looks almost the same, but we avoid the confusion of having two similar but slightly different assertion packages, and having to maintain our own package instead of using a commonly-used one. In the process, I found a few places where the tests should halt if an assertion fails, so I've made those cases (that I noticed) use "require" instead of "assert", and I've vendored the "require" package from testify alongside the already-present "assert" package. Signed-off-by: Aaron Lehmann <[email protected]>

aaronlehmann · 2017-04-14T19:04:57Z

Updated the PR to fully replace testutil/assert.

unclejack · 2017-04-14T19:55:33Z

Wasn't testify horribly broken? I recall testify was rejected a while ago as well.

unclejack · 2017-04-14T20:21:51Z

I believe @icecrime has looked into this the last time testify was being discussed.

aaronlehmann · 2017-04-14T20:50:52Z

How is it broken? Why is it better to have our own reimplementation?

unclejack · 2017-04-14T20:57:45Z

@aaronlehmann: I'm not saying we should have our own implementation. It's just that I recall there were objections to using testify. Some issues were brought up related to some bugs in testify. I'll look this up on the tracker and post here.

unclejack · 2017-04-14T21:10:57Z

@aaronlehmann: #10610 (comment) was the issue where some problems related to testify were brought up. I haven't used that code in a few years and I haven't checked if it's in a better condition since. My concern was mostly related to avoiding the package(s) if they have bugs/code quality issues. I don't have a strong opinion beyond that.

aaronlehmann · 2017-04-14T21:14:27Z

That PR seems to be a bug fix opened by @icecrime. It was merged more than two years ago.

I don't have a particular attachment to testify, though we use it in swarmkit and haven't encountered any bugs. However, I'm troubled that we've implemented our own assert package that's subtly incompatible. I'd strongly recommend we switch to testify unless anyone knows of outstanding bugs or quality issues.

vieux · 2017-04-16T15:29:35Z

as @aaronlehmann said, it seems like the issue in testify has been fixed upstream. I don't see a reason to not merge this one. LGTM

vdemeester

LGTM 👼

dnephin

LGTM

dnephin · 2017-04-17T14:43:17Z

+	assert.Equal(t, expected, b.image)
+	assert.Equal(t, expected, b.from.ImageID())
+	assert.Len(t, b.buildArgs.GetAllAllowed(), 0)
+	assert.Len(t, b.buildArgs.GetAllMeta(), 1)


I think this is a good example of an inconsistency in the design of testify.
assert.Len(), assert.Contains(), and assert.EqualError() have actual as the first arg, and expected as the second.

The args for assert.Equal(t, expected, actual) are in the opposite order. They should have been assert.Equal(t, actual, expected) to be consistent with the other assertions.

However I don't think that's enough of a reason to keep our own implementation.

tonistiigi · 2017-04-17T15:05:22Z

LGTM

Remove pkg/testutil/assert in favor of testify

GordonTheTurtle added the status/0-triage label Apr 13, 2017

dnephin added status/2-code-review and removed status/0-triage labels Apr 13, 2017

aaronlehmann force-pushed the testify branch from bd537ed to 82cd959 Compare April 14, 2017 00:01

aaronlehmann force-pushed the testify branch from 82cd959 to f28fb50 Compare April 14, 2017 01:32

vieux added rebuild/powerpc labels Apr 14, 2017

GordonTheTurtle removed rebuild/powerpc labels Apr 14, 2017

vieux added rebuild/janky labels Apr 14, 2017

GordonTheTurtle removed rebuild/janky labels Apr 14, 2017

vdemeester requested review from AkihiroSuda, dnephin and vdemeester April 14, 2017 12:47

aaronlehmann force-pushed the testify branch from f28fb50 to 64a5f19 Compare April 14, 2017 17:06

aaronlehmann force-pushed the testify branch from 64a5f19 to 6052f2b Compare April 14, 2017 19:04

aaronlehmann changed the title ~~[WIP] Remove pkg/testutil/assert in favor of testify~~ Remove pkg/testutil/assert in favor of testify Apr 14, 2017

AkihiroSuda added the rebuild/z label Apr 15, 2017

GordonTheTurtle removed the rebuild/z label Apr 15, 2017

vdemeester approved these changes Apr 16, 2017

View reviewed changes

dnephin approved these changes Apr 17, 2017

View reviewed changes

dnephin merged commit 1eec7b5 into moby:master Apr 17, 2017

dnephin mentioned this pull request Apr 17, 2017

Unit tests for cli/command/image package #32248

Merged

11 tasks

dnephin added a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#32614 from aaronlehmann/testify

31639e0

Remove pkg/testutil/assert in favor of testify

thaJeztah mentioned this pull request Jun 26, 2017

Add a LastTagTime for images #31497

Merged

Conversation

aaronlehmann commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin commented Apr 13, 2017

Uh oh!

dnephin commented Apr 14, 2017

Uh oh!

aaronlehmann commented Apr 14, 2017

Uh oh!

dnephin commented Apr 14, 2017

Uh oh!

aaronlehmann commented Apr 14, 2017

Uh oh!

vieux commented Apr 14, 2017

Uh oh!

aaronlehmann commented Apr 14, 2017

Uh oh!

vieux commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronlehmann commented Apr 14, 2017

Uh oh!

aaronlehmann commented Apr 14, 2017

Uh oh!

unclejack commented Apr 14, 2017

Uh oh!

unclejack commented Apr 14, 2017

Uh oh!

aaronlehmann commented Apr 14, 2017

Uh oh!

unclejack commented Apr 14, 2017

Uh oh!

unclejack commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronlehmann commented Apr 14, 2017

Uh oh!

vieux commented Apr 16, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin Apr 17, 2017

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Apr 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

aaronlehmann commented Apr 13, 2017 •

edited

Loading

vieux commented Apr 14, 2017 •

edited

Loading

unclejack commented Apr 14, 2017 •

edited

Loading