Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

virtcontainers: Add to bridges unit test #2828

Merged

Conversation

cmaf
Copy link
Contributor

@cmaf cmaf commented Jul 13, 2020

Add function that creates new bridges to increase unit test coverage
for virtcontainers/types/bridges.

Fixes #2827

Signed-off-by: Chelsea Mafrica [email protected]

@cmaf cmaf added needs-review Needs to be assessed by the team. wip Work in Progress (PR incomplete - needs more work or rework) labels Jul 13, 2020
@auto-comment
Copy link

auto-comment bot commented Jul 13, 2020

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!

@cmaf
Copy link
Contributor Author

cmaf commented Jul 13, 2020

I am still adding to this for more coverage for this unit test but a review is appreciated.

@cmaf
Copy link
Contributor Author

cmaf commented Jul 13, 2020

/test-ubuntu

@codecov
Copy link

codecov bot commented Jul 13, 2020

Codecov Report

Merging #2828 into master will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2828      +/-   ##
==========================================
+ Coverage   50.51%   50.65%   +0.14%     
==========================================
  Files         118      118              
  Lines       17331    17331              
==========================================
+ Hits         8754     8779      +25     
+ Misses       7503     7478      -25     
  Partials     1074     1074              

@cmaf cmaf force-pushed the unittest-virtcontainers-bridges branch from 5d14b6d to 50a4015 Compare July 13, 2020 22:32
@cmaf
Copy link
Contributor Author

cmaf commented Jul 13, 2020

/test-ubuntu

@cmaf cmaf removed the wip Work in Progress (PR incomplete - needs more work or rework) label Jul 13, 2020
@cmaf cmaf requested a review from jodh-intel July 14, 2020 19:58
Copy link
Member

@jcvenegas jcvenegas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions but it looks good

@cmaf cmaf force-pushed the unittest-virtcontainers-bridges branch from 50a4015 to 3f9d700 Compare July 15, 2020 17:55
@cmaf
Copy link
Contributor Author

cmaf commented Jul 15, 2020

@jcvenegas I updated the test. I added a test for the default bridge and I think I hit 100% coverage for this specific one.

Should I add a test for every parameter? I left ID as an empty string but I could test it, and I only test maxCapacity for the default bridge.

@cmaf
Copy link
Contributor Author

cmaf commented Jul 15, 2020

/test-ubuntu

Add function that creates new bridges to increase unit test coverage
for virtcontainers/types/bridges. Also adds test for address formats.

Fixes kata-containers#2827

Signed-off-by: Chelsea Mafrica <[email protected]>
@cmaf cmaf force-pushed the unittest-virtcontainers-bridges branch from 3f9d700 to e71b05b Compare July 15, 2020 18:20
@cmaf
Copy link
Contributor Author

cmaf commented Jul 15, 2020

/test-ubuntu

Copy link
Contributor

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cmaf.

lgtm

Thoughts: The NewBridge() function is a slightly strange one as it doesn't return an error. That makes it more difficult to unit test since the function itself cannot determine if the returned bridge is valid (which is unfortunate). However, a quick look at the code shows that if you changed NewBridge() to return an error, that would result in a fair bit of rework since the functions that call NewBridge() themselves don't return errors 😭

@jodh-intel jodh-intel added the port-to-2.0 PRs that need to be ported to kata 2.0-dev branch label Jul 16, 2020
@jodh-intel
Copy link
Contributor

@cmaf - could you port this to the https://github.com/kata-containers/kata-containers/tree/2.0-dev branch too please.

@cmaf cmaf added wip Work in Progress (PR incomplete - needs more work or rework) and removed wip Work in Progress (PR incomplete - needs more work or rework) labels Jul 16, 2020
@cmaf
Copy link
Contributor Author

cmaf commented Jul 28, 2020

Ported to 2.0 with 423

@cmaf cmaf removed the port-to-2.0 PRs that need to be ported to kata 2.0-dev branch label Jul 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-review Needs to be assessed by the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase unit test coverage for virtcontainers
4 participants