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

Ensure tests clean their tempfiles#496

Merged
bergwolf merged 3 commits intokata-containers:masterfrom
grahamwhaley:20180713_clean_tests
Jul 24, 2018
Merged

Ensure tests clean their tempfiles#496
bergwolf merged 3 commits intokata-containers:masterfrom
grahamwhaley:20180713_clean_tests

Conversation

@grahamwhaley
Copy link
Contributor

A number of tests were not cleaning up after themselves, which left
a bunch of files in the TMPDIR.
Ensure we remove all the tmpfiles and dirs we create.

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 162131 KB
Proxy: 5034 KB
Shim: 8978 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007220 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 18, 2018

Build succeeded (third-party-check pipeline).

@sboeuf
Copy link

sboeuf commented Jul 18, 2018

@grahamwhaley CI error:

ERROR: No "Fixes" found

@grahamwhaley
Copy link
Contributor Author

Hmm, but it is in the second commit.... let me check....

@sboeuf
Copy link

sboeuf commented Jul 18, 2018

@grahamwhaley you need it for every commit. That's fine to refer the same issue from different commit.

@chavafg
Copy link
Contributor

chavafg commented Jul 18, 2018

I don't think the Fixes: is needed for every commit, but I see something weird here: the checkcommits tool says that it found 3 commits, while the PR contains 4.

Running checkcommits version 0.0.1 (commit 805092aeeeb8897b355bbcb2b174ff3d5dda59ae)
Defaulting commit to HEAD
Defaulting branch to master
Found 3 commits between commit HEAD and branch master
ERROR: No "Fixes" found
ERROR: checkcommits failed. See the document below for help on formatting
commits for the project.

bug in the checkcommits tool?

@grahamwhaley
Copy link
Contributor Author

Err.... no you do not.
From: https://github.com/kata-containers/tests/tree/master/cmd/checkcommits#overview
checking for at least one "Fixes #XXX" entry in the commits
and from https://github.com/kata-containers/community/blob/master/CONTRIBUTING.md#general-format:

A "Fixes #XXX" comment listing the GitHub issue this change resolves.
This comment is required for the main patch in a sequence. See the following examples.

I've never 'Fixes' every commit - just normally the last one in the sequence :-)
Maybe that is the thing here - it is not the first or last commit - but, I've never seen that fail before.
I'll have a look

@grahamwhaley
Copy link
Contributor Author

@chavafg - yeah, hmm, that could be a bug in the tool or CI - there are indeed 4 patches. Heh, looks like we are going to have some checkcommits fun with no @jodh-intel :-)

@grahamwhaley grahamwhaley force-pushed the 20180713_clean_tests branch from 5e4a498 to 4206c61 Compare July 19, 2018 09:39
@grahamwhaley
Copy link
Contributor Author

OK, I think I know what happened!
Over on #475 I also fixed one of the issues in one of the commits here - so during the rebase/merge on the CI I think that commit gets dropped - hence the CI only sees 3 commits (the same happened to me here when I rebased to master - took a little bit of figuring out why I only then had 3 commits!).
It also happened to have been the commit with the Fixes in it :-) Lesson learnt - something to look out for in the future...
Rebased, reworeded, re-pushed...

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 162717 KB
Proxy: 5070 KB
Shim: 9078 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007096 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 19, 2018

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@grahamwhaley
Copy link
Contributor Author

Looks like static checks are fine, but the CIs suffered from the (unrelated I believe) kata-containers/tests#513
Rather than re-kick them all, will wait for any input from @chavafg on if that is a transient, persistent or sporadic failure mode.

@egernst
Copy link
Member

egernst commented Jul 23, 2018

@grahamwhaley @chavafg - we still waiting for a new rekick?

@grahamwhaley
Copy link
Contributor Author

Yep, let me do that (probably with a rebase) - let's see how the CI is feeling..

Graham Whaley added 3 commits July 23, 2018 17:32
Ensure we remove the tmpfile used for testing.

Signed-off-by: Graham Whaley <graham.whaley@intel.com>
We were defer removing the temporary config.json files
but not the tmpdir path we had created to store them in.
Expose that path out so we can defer removeall it.

Fixes: kata-containers#480

Signed-off-by: Graham Whaley <graham.whaley@intel.com>
Add a comment to clarify that the caller of
testRunContainerSetup() cleans up the tmpdir.

Signed-off-by: Graham Whaley <graham.whaley@intel.com>
@grahamwhaley grahamwhaley force-pushed the 20180713_clean_tests branch from 4206c61 to 50b445c Compare July 23, 2018 16:34
@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 160815 KB
Proxy: 5082 KB
Shim: 8972 KB

Memory inside container:
Total Memory: 2045968 KB
Free Memory: 2007540 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Jul 23, 2018

Build succeeded (third-party-check pipeline).

@bergwolf
Copy link
Member

bergwolf commented Jul 24, 2018

LGTM!

Looks like travis-ci is having issues with linux-ppc64le platform. Let's merge ignoring it since all the other CIs have passed.

Approved with PullApprove

@bergwolf bergwolf merged commit 8bdceb9 into kata-containers:master Jul 24, 2018
@sboeuf sboeuf added bug Incorrect behaviour stable-candidate labels Sep 12, 2018
@sboeuf
Copy link

sboeuf commented Sep 12, 2018

@grahamwhaley please backport this PR to stable branches if not present.

@sboeuf
Copy link

sboeuf commented Sep 12, 2018

Nevermind, it's already part of the stable branches.

zklei pushed a commit to zklei/runtime that referenced this pull request Jun 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Incorrect behaviour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants