Conversation
3d9440c to
544db9f
Compare
|
Thank you @2003Aditya - looks like it worked on Windows, so that's great.
In the commit message's You can squash and force-push the changes. |
integration/image/remove_test.go
Outdated
| assert.Check(t, is.ErrorContains(err, "unable to delete")) | ||
|
|
||
| _, err = apiClient.ImageRemove(ctx, "test:noexist", client.ImageRemoveOptions{}) | ||
| assert.Check(t, is.ErrorContains(err, "No such image")) |
There was a problem hiding this comment.
Not critical (can be a follow up) but perhaps we should update this to check the error type returned instead of string matching; we could still check if the error contains the image name (to double check the cause is indeed because of the image), but checking for the kind of error may be more relevant than the error message; similar to
moby/integration/container/stats_test.go
Line 91 in 27cefe6
4096215 to
e3ef220
Compare
|
Thank you for the update @2003Aditya ... please could you squash and force-push the commits, to make a single commit? (It'll be something like "git rebase -i master", and set the second commit to "s". Then "git push --force" to replace the existing commits.) It'd be good to pick up @thaJeztah's suggestion of checking the error types at the same time? |
integration/image/remove_test.go
Outdated
| fakecontext.WithDockerfile(`FROM busybox | ||
| ENV FOO bar`)) |
There was a problem hiding this comment.
I wasn't sure if whitespace at the start of the line in the Dockerfile would work - so tried it, and it was ok. But I got this warning, also worth fixing ..
1 warning found (use docker --debug to expand):
- LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 2)
d4c75fc to
5c1115c
Compare
integration/image/remove_test.go
Outdated
| _, err = apiClient.ImageRemove(ctx, imgID, client.ImageRemoveOptions{}) | ||
| assert.Check(t, is.ErrorContains(err, "unable to delete")) | ||
|
|
||
| _, err = apiClient.ImageRemove(ctx, "test:noexist", client.ImageRemoveOptions{}) | ||
| assert.ErrorType(t, err, cerrdefs.IsNotFound) |
There was a problem hiding this comment.
Thanks @2003Aditya - to implement the suggestion "to check the error type returned instead of string matching; we could still check if the error contains the image name (to double check the cause is indeed because of the image)" ... would need an error-type and a string check for both errors.
So, also using assert.Check so later checks still run if one fails, it'd be ...
| _, err = apiClient.ImageRemove(ctx, imgID, client.ImageRemoveOptions{}) | |
| assert.Check(t, is.ErrorContains(err, "unable to delete")) | |
| _, err = apiClient.ImageRemove(ctx, "test:noexist", client.ImageRemoveOptions{}) | |
| assert.ErrorType(t, err, cerrdefs.IsNotFound) | |
| _, err = apiClient.ImageRemove(ctx, imgID, client.ImageRemoveOptions{}) | |
| assert.Check(t, is.ErrorType(err, cerrdefs.IsConflict)) | |
| assert.Check(t, is.ErrorContains(err, "unable to delete "+stringid.TruncateID(imgID))) | |
| _, err = apiClient.ImageRemove(ctx, "test:noexist", client.ImageRemoveOptions{}) | |
| assert.Check(t, is.ErrorType(err, cerrdefs.IsNotFound)) | |
| assert.Check(t, is.ErrorContains(err, "No such image: test:noexist")) |
5c1115c to
313c3c7
Compare
… test framework Migrated TestAPIImagesDelete from the legacy integration-cli suite (docker_api_images_test.go) to the new integration test framework under integration/image/remove_test.go. This update: - Fixes ENV instruction syntax to use "ENV FOO=bar" - Adds error type check using errdefs.IsNotFound for cleaner assertions - Ensures consistent cleanup handling Signed-off-by: Aditya Mishra <mishraaditya675@gmail.com>
313c3c7 to
8f1134b
Compare
robmry
left a comment
There was a problem hiding this comment.
LGTM - thank you.
The TestBuildWithHugeFile timeout/failure is unrelated (the test would be skipped if rebased).
- What I did
Migrated the
TestAPIImagesDeletetest fromintegration-cli/docker_api_images_test.goto the new integration test framework underintegration/image/remove_test.go.- How to verify it
TESTFLAGS='-test.run TestAPIImagesDelete'make test-integration- Human readable description for the release notes
Migrate TestAPIImagesDelete from integration-cli to the new integration test framework.