Replace testify assertions with gotestyourself/assert #879

dnephin · 2018-02-14T22:40:26Z

We already use gotestyourself packages extensively in docker/cli and testify/assert has many issues, so replace testify with gotestyourself/assert (godoc).

The automated migration commit was done using gty-migrate-from-testify.

codecov-io · 2018-02-14T22:46:22Z

Codecov Report

Merging #879 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #879      +/-   ##
=========================================
+ Coverage   53.19%   53.2%   +0.01%     
=========================================
  Files         246     246              
  Lines       16021   16021              
=========================================
+ Hits         8522    8524       +2     
+ Misses       6929    6927       -2     
  Partials      570     570

thaJeztah · 2018-02-15T00:21:11Z

Are there any plans on picking up the effort to merge these projects? test-go/testify#13 (comment)

dnephin · 2018-02-15T03:38:19Z

At this point, I don't think so. At the time gotestyourself/assert wasn't merged yet and I thought there might be some shared effort. Shortly after it seemed like very little was happening with test-go. Now I think there's enough in gotestyourself/assert that it would be a lot less work to add a few more comparisons than to try and cleanup what's in testify.

vdemeester

LGTM 🦁

thaJeztah · 2018-02-15T09:05:10Z

cli/command/config/ls_test.go

-			assert.Equal(t, "foo", options.Filters.Get("name")[0])
-			assert.Equal(t, "lbl1=Label-bar", options.Filters.Get("label")[0])
+			assert.Check(t, is.Equal("foo", options.Filters.Get("name")[0]))
+			assert.Check(t, is.Equal("lbl1=Label-bar", options.Filters.Get("label")[0]))


Hm, I find assert.NoError(...) more easy on the eye than assert.Check(....); same as assert.Equal(..); for example:

assert.Equal(t, "lbl1=Label-bar", options.Filters.Get("label")[0])

vs

assert.Check(t, is.Equal("lbl1=Label-bar", options.Filters.Get("label")[0]))

The last one I have to read until position 20 (with indentation, position 32) to see what kind of check is done, and the is.Equal() add another level of nesting, which makes it quite a bit harder to read.

Basically assert.Something( enables me to "scan" the code, and at a glance grasp what steps are taken.

Was this a deliberate design choice in gotestyourself?

I find assert.NoError(...) more easy on the eye than assert.Check(....)

assert.NoError() (in testify) is almost always wrong because it only fails the test, but doesn't end execution immediately. If you don't end execution immediately on an error you will likely hit a panic on a following line. This is one of the many issues that should be fixed.

gotestyourself does have assert.NilError(). The automated migration tool preserves the existing behaviour so we ended up with a few assert.Check(t, err), but these should likely be converted to assert.NilError(t, err) in a follow up.

Was this a deliberate design choice in gotestyourself?

Yes, gotestyourself/gotest.tools#49 (comment).

I think we should add more assert.Something(), but I want to make sure we add the right ones and don't clutter the interface with infrequently used, or weak assertions.

assert.Check(t, is.Equal()) is an interesting case. We need a way to support assertions that use both T.Fail and T.FailNow. There are only so many ways this can be done. testify solved it with code generation and using a separate package. gotestyourself solves it by splitting the failure (assert.Check vs assert.Assert) from the comparison.

The convenience assertions added to the assert package all use FailNow because that should be the more common case. We could add a assert.CheckEqual() but I don't really see that as an improvement over assert.Check(t, isEqual()). Using assert.Equal() (which already exists) should be correct in most cases.

Maybe then you nay use testify/require instead? I think that require.NoError() terminates the execution.

Yes, you could use require.NoError(), but this PR should make it clear that it's too easy to do the wrong thing with testify. It is just one of the many problems with testify. require should be called assert, and assert should be something like "check".

gotestyourself makes it a lot easier to do the right thing by making it the most convenient and fixing the terminology.

@thaJeztah @silvin-lubecki

gotestyourself/gotest.tools#71 adds some more top level asserts for handling errors.

@dnephin looking good! I think we should move this forward (sorry for stalling this). Looks like this'll need a massive rebase (or was it all done automated?)

It was mostly automated, so it should be easy to just re-run that automation.

and fix some tests Signed-off-by: Daniel Nephin <dnephin@docker.com>

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Fix tests that failed when using cmp.Compare() internal/test/testutil/assert InDelta Fix DeepEqual with kube metav1.Time Convert some ErrorContains to assert Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin · 2018-03-06T00:50:11Z

rebased and green

silvin-lubecki

LGTM

thaJeztah · 2018-03-06T13:42:00Z

cli/command/bundlefile/bundlefile_test.go

@@ -37,7 +38,7 @@ func TestLoadFileSyntaxError(t *testing.T) {
 	}`)

 	_, err := LoadFile(reader)
-	assert.EqualError(t, err, "JSON syntax error at byte 37: invalid character 'u' looking for beginning of value")
+	assert.Check(t, is.Error(err, "JSON syntax error at byte 37: invalid character 'u' looking for beginning of value"))


Is it possible to automate changing these to assert.Error() (gotestyourself/gotest.tools#71), or do you want to keep that for a follow-up ?

It's possible to automate the change, but it's not the same as gotestyourself/gotest.tools#71.

The current automated migration preserves behaviour, so testify/require -> Assert, testify/assert -> Check.

I can write another migration that converts specific checks to the canonical Assert, but that is a change in behaviour (Fail -> FailNow), so I'd like to do that in a separate PR so that we can more carefully review the change.

thaJeztah

okay let’s merge what we have now, and improve in follow ups; thanks!

LGTM

dnephin requested review from vdemeester and thaJeztah February 14, 2018 22:40

GordonTheTurtle added the status/0-triage label Feb 14, 2018

vdemeester approved these changes Feb 15, 2018

View reviewed changes

vdemeester added status/2-code-review and removed status/0-triage labels Feb 15, 2018

thaJeztah reviewed Feb 15, 2018

View reviewed changes

Update some assertions.

93615dd

and fix some tests Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin force-pushed the update-assertions branch from 9157aa4 to b144de7 Compare March 6, 2018 00:32

dnephin added 3 commits March 5, 2018 19:41

Replace testify vendor with updated gotestyourself

5ef8835

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Automated migration

39c2ca5

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Post migration fixes

5155cda

Fix tests that failed when using cmp.Compare() internal/test/testutil/assert InDelta Fix DeepEqual with kube metav1.Time Convert some ErrorContains to assert Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin force-pushed the update-assertions branch from b144de7 to 5155cda Compare March 6, 2018 00:41

silvin-lubecki approved these changes Mar 6, 2018

View reviewed changes

thaJeztah reviewed Mar 6, 2018

View reviewed changes

thaJeztah approved these changes Mar 6, 2018

View reviewed changes

thaJeztah merged commit a0b19f0 into docker:master Mar 6, 2018

GordonTheTurtle added this to the 18.04.0 milestone Mar 6, 2018

dnephin deleted the update-assertions branch March 6, 2018 16:21

thaJeztah mentioned this pull request Apr 10, 2018

[18.03] backport content trust tests, windows, test-refactoring docker-archive/docker-ce#504

Merged

thaJeztah added process/cherry-picked area/testing labels Apr 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace testify assertions with gotestyourself/assert #879

Replace testify assertions with gotestyourself/assert #879

dnephin commented Feb 14, 2018

codecov-io commented Feb 14, 2018 •

edited

Loading

thaJeztah commented Feb 15, 2018

dnephin commented Feb 15, 2018

vdemeester left a comment

thaJeztah Feb 15, 2018

dnephin Feb 15, 2018

silvin-lubecki Feb 16, 2018

dnephin Feb 16, 2018

dnephin Mar 4, 2018

thaJeztah Mar 5, 2018

dnephin Mar 5, 2018

dnephin commented Mar 6, 2018

silvin-lubecki left a comment

thaJeztah Mar 6, 2018

dnephin Mar 6, 2018

thaJeztah left a comment

Replace testify assertions with gotestyourself/assert #879

Replace testify assertions with gotestyourself/assert #879

Conversation

dnephin commented Feb 14, 2018

codecov-io commented Feb 14, 2018 • edited Loading

Codecov Report

thaJeztah commented Feb 15, 2018

dnephin commented Feb 15, 2018

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin commented Mar 6, 2018

silvin-lubecki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 14, 2018 •

edited

Loading