-
Notifications
You must be signed in to change notification settings - Fork 373
virtcontainers: Add unit test for types/container.go #2857
virtcontainers: Add unit test for types/container.go #2857
Conversation
Thank you for raising your pull request. Please note that the main development of Kata Containers has moved to the 2.0-dev branch of https://github.com/kata-containers/kata-containers repository. The kata-containers/runtime repository is kept for 1.x release maintenance. Please check twice if your change should go to the 2.0-dev branch directly. If it is strongly required for adding the change to Kata Containers 1.x releases, please ping @kata-containers/runtime to assign a dedicated developer to be responsible for porting the change to 2.0-dev branch. Thanks! |
/test-ubuntu |
Codecov Report
@@ Coverage Diff @@
## master #2857 +/- ##
=======================================
Coverage 51.05% 51.06%
=======================================
Files 118 118
Lines 17364 17372 +8
=======================================
+ Hits 8866 8871 +5
- Misses 7419 7422 +3
Partials 1079 1079 |
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 - just a few questions.
71c4298
to
35b8155
Compare
@merwick Thanks for the review, I applied these changes. I also added a |
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
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 @cmaf. Just one suggestion.
} | ||
|
||
ok := state.Valid() | ||
assert.Equal(t, ok, expected) |
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.
I'd consider adding a string at the end of this call containing state
and expected
) to aid with debugging. It's a good habit to get into, particularly when trying to debug "remote failures" (those annoying tests that work locally but fail in the CI ;-)
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.
@jodh-intel thanks for the comment, I added a message here
/test |
Add unit tests for types/container.go. Tests were adapted from sandbox_test.go since ContainerState is a sandbox state structure and the transition tests are the same. Fixes kata-containers#2856 Signed-off-by: Chelsea Mafrica <[email protected]>
35b8155
to
e8e1124
Compare
I'm also applying these changes to the 2.x PR, #452 /test |
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 @cmaf.
lgtm
Add unit tests for types/container.go. Tests were adapted from
sandbox_test.go since ContainerState is a sandbox state structure and
the transition tests are the same.
Fixes #2856
Signed-off-by: Chelsea Mafrica [email protected]