tests: migrate assertions to be more modern by cyphar · Pull Request #48915 · moby/moby

cyphar · 2024-11-21T09:24:21Z

Quite a lot of tests (especially in integration-cli) use boolean assertions which don't provide any useful information about why a comparison failed. This is already kind of annoying when debugging a failing test, but when running the integration suite on an actual machine (where the source code parsing doesn't work) this makes it almost impossible to figure out what the actual error is.

A lot of this is due to limitations of gotest.tools (see gotestyourself/gotest.tools#147 for instance) but we can work around most of these issues.

NOTE: I've only tackled the "easy" cases for now. All of the transformations were done using gofmt -r or golang.org/x/tools/cmd/eg, which should make it easier to verify that the tests weren't subtly broken by these changes.

Future work:

Fix all of the assertions that have help messages (these are less critical because usually the message gives you some kind of information). Unfortunately, gofmt cannot handle these automatically, and we would need to run gofmt for every number of arguments used. We could use sed but this could lead to bugs, and eg seems to be able to do this but it is incredibly poorly documented.
Migrate assert.Assert(t, errors.Is(...)) to assert.ErrorIs or is.ErrorIs.
Migrate assert.Assert(t, is.Contains(err.Error(), ...) to assert.ErrorContains.
Fix inverse comparisons. integration-cli/checker doesn't help with this entirely (see Issues implementing a "Not" comparison gotestyourself/gotest.tools#147) -- we will instead need to create a new cmpext package that will let us create a generic inverter that has useful error messages. This will then let us tackle these issues:
- assert.Assert(t, a != nil)
- assert.Assert(t, !strings.Contains(...))
- ...

cyphar · 2024-11-21T09:27:03Z

I'll work on the cmpext stuff if there is any interest in taking this kind of patchset.

thaJeztah · 2024-11-21T10:18:14Z

Looks like potentially flaky test; I see this one was mentioned in #48609 as being flaky (and possibly to be fixed with that one?) cc @robmry FYI

=== Failed
=== FAIL: amd64.integration.networking TestAccessPublishedPortFromHost/userland-proxy=true/IPv6=false (1.09s)
    port_mapping_linux_test.go:392: assertion failed: error is not nil: Get "http://127.0.0.1:1234": EOF
    --- FAIL: TestAccessPublishedPortFromHost/userland-proxy=true/IPv6=false (1.09s)

thaJeztah

Thanks for working on this! I think I had some branches stashed with some of these changes, but never got to complete them 🤗

changes so far LGTM (assuming CI passes)

I see you left some TODO's, notably;

Fix all of the assertions that have help messages

I'd be fine merging this one as-is, and work on remaining ones as follow-up(s); perhaps some of those changes (if we can't come with a "smart" solution to find/replace) would be just a matter of some elbow-grease to go through them? (should be something we'd only have to do once to bring it up to speed).

thaJeztah · 2024-11-21T10:04:32Z

-	assert.Assert(c, strings.Contains(strings.TrimSpace(out), id))
+	assert.Assert(c, is.Contains(strings.TrimSpace(out), id))


Not necessarily for this PR, unless you need to make more changes; looks like these strings.Trimspace() calls here (and couple of other places below?) are redundant, as we're checking if it contains id

thaJeztah · 2024-11-21T10:07:07Z

@@ -2282,7 +2283,7 @@ func (s *DockerCLIRunSuite) TestRunAllowPortRangeThroughExpose(c *testing.T) {
 func (s *DockerCLIRunSuite) TestRunExposePort(c *testing.T) {
 	out, _, err := dockerCmdWithError("run", "--expose", "80000", "busybox")
 	assert.Assert(c, err != nil, "--expose with an invalid port should error out")


Note to self: these blanket "any error should do" cases keep catching my eye. I may have a go at finding these to see if we can be more specific (admitted, the output check below should probably prevent catching random unrelated errors, but still)

Yeah these checks really should be ErrorsIs or at the very least ErrorContains but unfortunately that migration can't be easily automated. Luckily there's fewer than 100 of them, so we could migrate them in one patch:

% git grep "assert.\(Assert\|Check\)(.*err.* != nil" | wc -l 87

thaJeztah · 2024-11-21T10:11:09Z

-	assert.Assert(c, strings.Contains(result.Stdout(), "foo=abc"))
+	assert.Assert(c, is.Contains(result.Stdout(), "foo=abc"))
 	result = cli.DockerCmd(c, "run", "--rm", imgName, "cat", "/out")
 	assert.Assert(c, !strings.Contains(result.Stdout(), "foo"))


Still looking for more elegant solutions to these (negative matches); we could probably consider putting result.Stdout() as part of a custom failure message to help (definitely not needed for this PR though)

edit: ah; I see you mentioned this in your PR description as well

Fix inverse comparisons. integration-cli/checker doesn't help with this entirely

Yeah, the trick is that we need to create a custom type that wraps assert.BoolOrComparison which then inverts Success as well as providing a wrapped FormatMessage that describes that the comparison was inverted. I suspect the wrapped description is going to be quite janky (it would probably look like the suggestion that the author of gotest.tools proposed in gotestyourself/gotest.tools#147 (comment)). Unfortunately, FormatMessage is technically internal to gotest.tools so it could break in the future (but the author suggested doing it this way, so I'm not sure).

Since it seems you'd be happy with a patch like that, I can try to cook one up.

Yeah, a custom helper could work still, but perhaps we could still look if we could contribute a patch to gotest.tools that would be acceptable to their maintainers 🤔

For the formatting, we could still go the manual way;

assert.Assert(c, !strings.Contains(result.Stdout(), "foo"), "Expected: %q to contain %q", result.Stdout(), "foo")

Which (if all of these are in the same (integration-cli) package could be a local helper I guess;

func assertNotContains(t testing.T, val, expected string) { t.Helper() assert.Assert(t, !strings.Contains(val, expected), "Expected %q not to contain %q", val, expected) }

That would be the simplest option if we want to avoid over-complicating things. I'll go with that version now and we can do the more complicated version in the future...

thaJeztah · 2024-11-21T12:12:39Z

@cyphar let me know if you want to add more changes to this PR before merging, or if you're good with bring the current set of changes in already. I think the current set of changes are already an improvement, so (from my perspective) the remaining ones are not a blocker for bringing those in already.

cyphar · 2024-11-21T12:37:41Z

I'd like to fix the errors.Is and strings.Contains(err.Error(), ...) checks but all of the other stuff can be done in later PRs IMHO.

cyphar · 2024-11-22T01:23:15Z

A proper ErrorsIs might actually be more involved because a lot of the cases that look ripe for conversion might not actually wrap the error properly or the test might be checking something subtly different. So I've just gone with the "dumb" version of the patch where we only convert cases that are clearly correct (so I've skipped converting any errors.As code as well as more complicated checks).

Migrated using find . -type f -name "*_test.go" | xargs gofmt -w \ -r "assert.Check(t, strings.Contains(a, b)) -> assert.Check(t, is.Contains(a, b))" find . -type f -name "*_test.go" | xargs gofmt -w \ -r "assert.Assert(t, strings.Contains(a, b)) -> assert.Assert(t, is.Contains(a, b))" Using a boolean in assert.Assert or assert.Check results in error messages that don't contain the actual problematic string, and when running the integration suite on an actual machine (where the source code parsing doesn't work) this makes it almost impossible to figure out what the actual error is. Signed-off-by: Aleksa Sarai <[email protected]>

Unfortunately, gofmt doesn't know about types so it was necessary to find all of the err == nil statements through trial and error. Note that there is no is.NilError, so for assert.Check(t, err == nil) we need to switch to just doing assert.Check(t, err). If err is an error type, this is equivalent (and there isn't another trivial way of doing it). Here are the full set of rules used: Generic "err == nil": find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, err == nil) -> assert.NilError(t, err)" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, err == nil) -> assert.Check(t, err)" Generic, but with a different variable name: find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, sr.err == nil) -> assert.NilError(t, sr.err)" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, sr.err == nil) -> assert.Check(t, sr.err)" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, err2 == nil) -> assert.NilError(t, err2)" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, err2 == nil) -> assert.Check(t, err2)" JSON-related error assertions: find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, json.Unmarshal(a, b) == nil) -> assert.NilError(t, json.Unmarshal(a, b))" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, json.Unmarshal(a, b) == nil) -> assert.Check(t, json.Unmarshal(a, b))" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, json.NewDecoder(a).Decode(b) == nil) -> assert.NilError(t, json.NewDecoder(a).Decode(b))" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, json.NewDecoder(a).Decode(b) == nil) -> assert.Check(t, json.NewDecoder(a).Decode(b))" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, json.NewEncoder(a).Encode(b) == nil) -> assert.NilError(t, json.NewEncoder(a).Encode(b))" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, json.NewEncoder(a).Encode(b) == nil) -> assert.Check(t, json.NewEncoder(a).Encode(b))" Process-related error assertions: find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, a.Start() == nil) -> assert.NilError(t, a.Start())" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, a.Start() == nil) -> assert.Check(t, a.Start())" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, a.Kill() == nil) -> assert.NilError(t, a.Kill())" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, a.Kill() == nil) -> assert.Check(t, a.Kill())" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, a.Signal(b) == nil) -> assert.NilError(t, a.Signal(b))" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, a.Signal(b) == nil) -> assert.Check(t, a.Signal(b))" waitInspect: find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, waitInspect(a, b, c, d) == nil) -> assert.NilError(t, waitInspect(a, b, c, d))" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, waitInspect(a, b, c, d) == nil) -> assert.Check(t, waitInspect(a, b, c, d))" File closing error assertions: find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, a.Close() == nil) -> assert.NilError(t, a.Close())" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, a.Close() == nil) -> assert.Check(t, a.Close())" mount.MakeRShared: find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, mount.MakeRShared(a) == nil) -> assert.NilError(t, mount.MakeRShared(a))" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, mount.MakeRShared(a) == nil) -> assert.Check(t, mount.MakeRShared(a))" daemon.SwarmLeave: find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, d.SwarmLeave(a, b, c) == nil) -> assert.NilError(t, d.SwarmLeave(a, b, c))" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, d.SwarmLeave(a, b, c) == nil) -> assert.Check(t, d.SwarmLeave(a, b, c))" os.MkdirAll: find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, os.MkdirAll(a, b) == nil) -> assert.NilError(t, os.MkdirAll(a, b))" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, os.MkdirAll(a, b) == nil) -> assert.Check(t, os.MkdirAll(a, b))" Signed-off-by: Aleksa Sarai <[email protected]>

If a values is non-nil when we don't expect it, it would be quite helpful to get an error message explaining what happened. find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Assert(t, a == nil) -> assert.Assert(t, is.Nil(a))" find . -type f -name "*_test.go" | \ xargs gofmt -w -r "assert.Check(t, a == nil) -> assert.Check(t, is.Nil(a))" Signed-off-by: Aleksa Sarai <[email protected]>

If we have an error type that we're checking a substring against, we should really be checking using ErrorContains to indicate the right semantics to assert. Mostly done using these transforms: find . -type f -name "*_test.go" | \ xargs gofmt -w -r 'assert.Assert(t, is.ErrorContains(e, s)) -> assert.ErrorContains(t, e, s)' find . -type f -name "*_test.go" | \ xargs gofmt -w -r 'assert.Assert(t, is.Contains(err.Error(), s)) -> assert.ErrorContains(t, err, s)' find . -type f -name "*_test.go" | \ xargs gofmt -w -r 'assert.Check(t, is.Contains(err.Error(), s)) -> assert.Check(t, is.ErrorContains(err, s))' As well as some small fixups to helpers that were doing strings.Contains explicitly. Signed-off-by: Aleksa Sarai <[email protected]>

There were a handful of direct checks against errors.Is that can be translated to assert.ErrorIs without too much thought. Unfortunately there are a load of other examples where ErrorIs probably makes sense but would require testing whether this subtly breaks the test. These transformations were done by hand. Signed-off-by: Aleksa Sarai <[email protected]>

cyphar · 2024-11-22T13:05:14Z

@thaJeztah I think this should be okay for now.

thaJeztah

LGTM, thank you!

thaJeztah · 2024-11-22T16:46:44Z

 			} else {
 				_, err := os.Stat(filepath.Join(r, "sub/subfile"))
-				assert.Assert(t, is.ErrorContains(err, ""))
+				assert.ErrorContains(t, err, "")


For others; I was wondering if there were some of these we kept in the assert.Assert(.. is...) form if they were considered for "this could be a assert.Check instead to get more assertions on failure, but I don't think that's a strong motivator to keep that format.

cyphar · 2024-11-23T07:58:13Z

Once I finish doing the rest of the mitrations, it might make sense to add these as CI lints (because most of these are automated we can even provide a diff). That depends on whether I can get eg to handle variadic functions nicely, so we can get lints working nicely regardless of the number of arguments (and eg is type aware, which is something we almost certainly want for an automated lint)...

thaJeztah approved these changes Nov 21, 2024

View reviewed changes

thaJeztah added status/2-code-review area/testing kind/refactor PR's that refactor, or clean-up code labels Nov 21, 2024

thaJeztah added this to the 28.0.0 milestone Nov 21, 2024

cyphar force-pushed the tests-assert-modern branch 4 times, most recently from 140632d to 1968727 Compare November 22, 2024 04:31

cyphar added 5 commits November 22, 2024 23:59

cyphar force-pushed the tests-assert-modern branch from 1968727 to 557e4ed Compare November 22, 2024 12:59

cyphar marked this pull request as ready for review November 22, 2024 13:04

cyphar requested a review from tonistiigi as a code owner November 22, 2024 13:04

thaJeztah approved these changes Nov 22, 2024

View reviewed changes

thaJeztah reviewed Nov 22, 2024

View reviewed changes

neersighted approved these changes Nov 22, 2024

View reviewed changes

thaJeztah merged commit ca0910c into moby:master Nov 22, 2024

cyphar deleted the tests-assert-modern branch November 23, 2024 07:56

		assert.Assert(c, strings.Contains(strings.TrimSpace(out), id))
		assert.Assert(c, is.Contains(strings.TrimSpace(out), id))

Conversation

cyphar commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyphar commented Nov 21, 2024

Uh oh!

thaJeztah commented Nov 21, 2024

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 21, 2024

Uh oh!

cyphar commented Nov 21, 2024

Uh oh!

cyphar commented Nov 22, 2024

Uh oh!

cyphar commented Nov 22, 2024

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyphar commented Nov 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cyphar commented Nov 21, 2024 •

edited

Loading

cyphar commented Nov 23, 2024 •

edited

Loading