-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
command: add tests for container diff and rename #5467
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! Left a couple of nits to make the tests a little tidier, but overall LGTM
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5467 +/- ##
==========================================
+ Coverage 59.74% 59.92% +0.17%
==========================================
Files 345 345
Lines 23431 23431
==========================================
+ Hits 13999 14040 +41
+ Misses 8458 8417 -41
Partials 974 974 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for contributing! I left comments/ suggestions inline; let me know what you think
@thaJeztah @laurazard Thank you for the code review:) I committed the suggested changes from GitHub UI and will now work on the code review comments. When this pull request is ready after all the required changes, I will rebase and sign to match the Contribution guidelines. |
83d3e1a
to
ed54f4e
Compare
ed54f4e
to
0d89372
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@laurazard @thaJeztah I added all the requested changes and squashed the git commits. |
@thaJeztah this LGTY? |
This commit adds tests for the commands docker diff and docker rename. Also, it creates the mock methods of the docker client ContainerDiff and ContainerRename so they can be used in the tests. For docker diff, it covers the cases that: - the command runs successfully - the client returns an error - the container id is empty For docker rename, it covers the cases that: - the command runs successfully - the container old name is empty - the container new name is empty - the client returns an error Co-authored-by: Laura Brehm <[email protected]> Signed-off-by: Stavros Panakakis <[email protected]>
0d89372
to
46b360b
Compare
@laurazard @thaJeztah I renamed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
sorry for the delay; I somehow forgot about this one 🙈
Thanks for contributing!! |
- What I did
This commit adds tests for the commands
docker diff
anddocker rename
. Also, it creates the mock methods of the docker clientContainerDiff
andContainerRename
so they can be used in the tests.For
docker diff,
it covers the cases that:For
docker rename
, it covers the cases that:- How I did it
I read all the test cases developed in
cli/command/container
directory and did the same fordiff.go
andrename.go
using afakeClient
and mocking the clientContainerDiff
andContainerRename
methods.- How to verify it
Run:
make test
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)