feat: Providing tests for ComposeDown
#50
Conversation
This provies the missing tests for PR Mcrich23#39
oxisto
force-pushed
the
feature/compose-down-not-stopping-containers
branch
from
d4a37ab to
5991999
Compare
oxisto
changed the title
ComposeDown
|
This looks great! Thank you so much. Going to do a quick code review right now. |
Mcrich23
left a comment
Mcrich23
left a comment
There was a problem hiding this comment.
After taking a deeper look, it mostly feels good, but I have some specific requests regarding tests to make sure our tests are robust and strong to bring into the future of Container-Compose.
| containers.count == 2, | ||
| "Expected 2 containers for \(project.name), found \(containers.count)") | ||
| #expect( | ||
| containers[0].status == .running, |
There was a problem hiding this comment.
Could you please make this check that all of these containers are running instead of just the first one?
There was a problem hiding this comment.
adjusted it, but somehow this fails on my machine now, since it can't start the Wordpress container, but that might be a different bug :(
There was a problem hiding this comment.
Interesting. Try manually restarting the container subsystem
| containers.count == 2, | ||
| "Expected 2 containers for \(project.name), found \(containers.count)") | ||
| #expect( | ||
| containers[0].status == .stopped, |
There was a problem hiding this comment.
Could you please make this check that all of these containers are stopped instead of just the first one?
There was a problem hiding this comment.
adjusted it, but somehow this fails on my machine now, since it can't start the Wordpress container, but that might be a different bug :(
| containers.count == 1, | ||
| "Expected 1 container with the name \(containerName), found \(containers.count)") | ||
| #expect( | ||
| containers[0].status == .running, |
There was a problem hiding this comment.
Could you please make this check that all of these containers are running instead of just the first one?
There was a problem hiding this comment.
There should only be one for this particular test-case :)
There was a problem hiding this comment.
But still, best practice would be all containers. Just in case there is a future where this changes.
There was a problem hiding this comment.
so I should do a loop from 0 to 1? I am not quite sure how you would imagine the perfect solution to be. free for any suggestions :)
There was a problem hiding this comment.
Maybe just do a filter for all that have a status of stopped and check that the count of that array is 0
Something like:
containers.filter { $0.status == .stopped }.count == 1
| containers.count == 1, | ||
| "Expected 1 container with the name \(containerName), found \(containers.count)") | ||
| #expect( | ||
| containers[0].status == .stopped, |
There was a problem hiding this comment.
Could you please make this check that all of these containers are stopped instead of just the first one?
There was a problem hiding this comment.
There should only be one for this particular test-case :)
3 participants
This PR adds a dynamic tests for
ComposeDownand incorporates the fix of #39 to correctly handle container_name.