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

virtcontainers: fix unit tests#193

Merged
sboeuf merged 3 commits intokata-containers:masterfrom
devimc:virtcontainers/fixUnitTests
Apr 9, 2018
Merged

virtcontainers: fix unit tests#193
sboeuf merged 3 commits intokata-containers:masterfrom
devimc:virtcontainers/fixUnitTests

Conversation

@devimc
Copy link
Copy Markdown

@devimc devimc commented Apr 5, 2018

Fix unit tests according with new changes introduced recently.
Use noopAgent in unit tests to add online fake resources.
Factorize configuration and hardware support for hotplugging block
devices into a single function and use that.

fixes #192

Signed-off-by: Archana Shinde archana.m.shinde@intel.com
Signed-off-by: Julio Montes julio.montes@intel.com

@devimc devimc added the review label Apr 5, 2018
@amshinde
Copy link
Copy Markdown
Member

amshinde commented Apr 5, 2018

Can you correct typo in commit message /nooAgent/noopAgent/

@devimc devimc force-pushed the virtcontainers/fixUnitTests branch from 5153ad2 to 65bb7ec Compare April 5, 2018 18:22
@devimc
Copy link
Copy Markdown
Author

devimc commented Apr 5, 2018

@amshinde done, thanks

@devimc devimc changed the title virtcontainers: container: fix TestContainerAddResources unit test virtcontainers: fix unit tests Apr 5, 2018
@devimc devimc force-pushed the virtcontainers/fixUnitTests branch 2 times, most recently from 89f8c69 to 62220a4 Compare April 5, 2018 18:44
}

if container.state.Fstype == "" || !container.state.HotpluggedDrive {
if container.state.Fstype != "" && container.state.HotpluggedDrive {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So you are actually checking here that the drive is not hotplugged, which changes what the test is supposed to verify.
I think you need to add a noopAgent for the pod and have the noopAgent advertise the block support for the test.

@devimc devimc force-pushed the virtcontainers/fixUnitTests branch from 62220a4 to 92fa5e9 Compare April 5, 2018 18:55
@amshinde
Copy link
Copy Markdown
Member

amshinde commented Apr 5, 2018

lgtm

Use noopAgent in unit tests to add online fake resources.
Fix unit tests according with new changes introduced recently.

fixes kata-containers#192

Signed-off-by: Julio Montes <julio.montes@intel.com>
@devimc devimc force-pushed the virtcontainers/fixUnitTests branch 2 times, most recently from 797bb07 to 16fd7bb Compare April 6, 2018 13:22
@sboeuf
Copy link
Copy Markdown

sboeuf commented Apr 6, 2018

@devimc CI not passing because of CNI unit tests.

@chavafg
Copy link
Copy Markdown
Contributor

chavafg commented Apr 6, 2018

kata-containers/tests#219 makes the CI to run the virtcontainers unit tests with root user only. But now I see a panic in the CI logs of the ubuntu 16.04 job. and an issue with the log-parser tool in the 17.10 job

}
}

func (c *Container) checkBlockDeviceSupport() bool {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is part of #138, why include it here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@amshinde ^^ ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@bergwolf Without this factorization we need to introduce changes in the noopAgent implementation to support capabilities related to block device support since this code was moved into hotplugDrive which we are testing here. I think this is not necessary since we anyways need to factorize and reuse in #138. So I have moved it here to avoid the unnecessary implementation changes for noopAgent.

Copy link
Copy Markdown
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

lgtm

@grahamwhaley
Copy link
Copy Markdown
Contributor

For the PID log fix that was btw...

@devimc devimc force-pushed the virtcontainers/fixUnitTests branch from 0e299a9 to 656e520 Compare April 9, 2018 17:22
amshinde and others added 2 commits April 9, 2018 15:49
Factorize configuration and hardware support for hotplugging block
devices into a single function and use that.

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
in order to make log-parser happy, mockcontainer must return
always a valid process with a fake PID, since log-parser checks
that PID value in the logs and it must be different to zero

Depends-on: github.com/kata-containers/tests#226

Signed-off-by: Julio Montes <julio.montes@intel.com>
@devimc devimc force-pushed the virtcontainers/fixUnitTests branch from 656e520 to dacc175 Compare April 9, 2018 20:50
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 9, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@bc83bf0). Click here to learn what that means.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #193   +/-   ##
=========================================
  Coverage          ?   66.47%           
=========================================
  Files             ?       70           
  Lines             ?     7535           
  Branches          ?        0           
=========================================
  Hits              ?     5009           
  Misses            ?     1986           
  Partials          ?      540
Impacted Files Coverage Δ
virtcontainers/pkg/vcmock/container.go 0% <0%> (ø)
virtcontainers/container.go 51.39% <50%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc83bf0...dacc175. Read the comment docs.

@amshinde
Copy link
Copy Markdown
Member

amshinde commented Apr 9, 2018

@sboeuf Can we merge this one?

@sboeuf sboeuf merged commit 5932803 into kata-containers:master Apr 9, 2018
@sboeuf sboeuf removed the review label Apr 9, 2018
@devimc devimc deleted the virtcontainers/fixUnitTests branch May 9, 2018 15:39
mcastelino pushed a commit to mcastelino/tests that referenced this pull request Jan 23, 2019
Virtcontainers unit tests only work if they are executed
using the root user.

Fixes clearcontainers#218.

Depends-on: github.com/kata-containers/runtime#193

Signed-off-by: Salvador Fuentes <salvador.fuentes@intel.com>
zklei pushed a commit to zklei/runtime that referenced this pull request Jun 13, 2019
ListProcesses returns a list of running processes inside
the container, this function should be called by the runtime
in the ps command implementation.

fixes kata-containers#193

Signed-off-by: Julio Montes <julio.montes@intel.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TestContainerAddResources fails

6 participants