Update integration test integration-cli/docker_cli_rmi_test.go with Assert
#16901
Conversation
| @@ -266,6 +236,12 @@ func (s *DockerSuite) TestRmiBlank(c *check.C) { | |||
| if !strings.Contains(out, "image name cannot be blank") { | |||
| c.Fatalf("Expected error message not generated: %s", out) | |||
| } | |||
| // Should have failed to delete '' image | |||
There was a problem hiding this comment.
seems forget to remove the
if err == nil {
}
if string.Contians{
}
if !string.Contains{
}
up there.
There was a problem hiding this comment.
You are right. 👍
WeiZhang555
force-pushed
the
rmi-inte
branch
2 times, most recently
from
d9e5817 to
c60e9bf
Compare
| c.Fatalf("failed to create a container: %s, %v", out, err) | ||
| } | ||
| // failed to create a container | ||
| c.Assert(err, checker.IsNil, check.Commentf("out: %s", out)) |
There was a problem hiding this comment.
I wonder if we can use dockerCmd instead of dockerCmdWithError here, so that the assert for err is no need any more.
There was a problem hiding this comment.
Completely true, should use dockerCmd.
There was a problem hiding this comment.
Do you mean that use
out, _ := dockerCmd("run", "-d", "busybox", "true")
to replace
out, _, err := dockerCmdWithError("run", "-d", "busybox", "true")
c.Assert(err, checker.IsNil, check.Commentf("out: %s", out))
If so, do you really think it's OK to ignore failure of docker run? I really doubt it...
There was a problem hiding this comment.
@WeiZhang555 yes it is ! You don't ignore failure of docker run, they are "handled" in dockerCmd (see https://github.com/docker/docker/blob/master/pkg/integration/dockerCmd_utils.go#L33, there is an Assert on the err 😉)
There was a problem hiding this comment.
I got your point, that's very clear.
Will update. :)
Part of moby#16756 Update integration-cli/docker_cli_rmi_test.go: Use Assert instead of condition judgement in test. Signed-off-by: Zhang Wei <zhangwei555@huawei.com>
|
@vdemeester refreshed per your suggestion, please help check if this is OK now, thank you :) |
| // Container is using image, should not be able to rmi | ||
| c.Assert(err, checker.NotNil) | ||
| // Container is using image, error message should contain errSubstr | ||
| c.Assert(out, checker.Contains, errSubstr, check.Commentf("Container: %q", cleanedContainerID)) |
There was a problem hiding this comment.
I would have kept the comment that was in c.Fatalf in a check.Commentf
There was a problem hiding this comment.
As we have discussed in #16854, comment above c.Assert statement will be printed out together with check.Commentf, do you still see a need to put comment in check.Commentf?
@vdemeester
|
Sorry couple of more comments and we should be good to go 😉 |
|
LGTM 🐼 |
WeiZhang555
changed the title
integration-cli/docker_cli_rmi_test.go with Assert
|
Any more progress ? |
|
LGTM |
Update integration test `integration-cli/docker_cli_rmi_test.go` with Assert
Part of #16756
Update integration-cli/docker_cli_rmi_test.go:
Use Assert instead of condition judgement in test.
Signed-off-by: Zhang Wei zhangwei555@huawei.com