Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

CI: Specify tmpdir for running golang unit tests #870

Merged
merged 2 commits into from
Oct 29, 2018

Conversation

jodh-intel
Copy link
Contributor

It is possible for the go-test.sh script to fail when attemping to
write to a file as it can already be closed. This problem seems to
relate to the golang testing package itself so work around the
problem by specifying an explicit temporary directory for testing to
create files in.

Fixes #869.

Signed-off-by: James O. D. Hunt james.o.hunt@intel.com

.ci/go-test.sh Outdated
@@ -10,6 +10,9 @@ set -e
script_name=${0##*/}
typeset -A long_options

# directory to use for temporary files
TMPDIR=${TMPDIR:-/tmp}
Copy link
Contributor

Choose a reason for hiding this comment

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

is mktemp -d overkill :) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

you mean specifically make a new tmp directory for the tests to use @marcov ? I could go for that.

Move the `go_test_flags` variable to the `main()` function to simplify
subsequent changes.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
It is possible for the `go-test.sh` script to fail when attemping to
write to a file as it can already be closed. This problem seems to
relate to the golang `testing` package itself so work around the problem
by specifying an explicit temporary directory for `testing` to create
files in.

Fixes kata-containers#869.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@jodh-intel
Copy link
Contributor Author

@marcov - good call - branch updated!

@marcov
Copy link
Contributor

marcov commented Oct 29, 2018

/test

@marcov
Copy link
Contributor

marcov commented Oct 29, 2018

lgtm

Approved with PullApprove

Copy link
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
obviously an improvement.
one question.

if [ "$1" = "html-coverage" ]; then
test_html_coverage
elif [ "$run_coverage" = yes ]; then
test_coverage
else
test_local
fi

[ -d "${golang_tmp}" ] && [ "${golang_tmp}" != "/" ] && rm -rf "${golang_tmp}"
Copy link
Contributor

Choose a reason for hiding this comment

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

as the script has a -e set, would this be safer as a shell TRAP to catch the case where the tests fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did consider that but thought it would be better to leave the tmpfiles in the case of test failures as they might help us diagnose what went wrong.

@chavafg chavafg merged commit 651375d into kata-containers:master Oct 29, 2018
@bergwolf
Copy link
Member

@jodh-intel What happens to the commit? I can't find the corresponding code in tests master and I'm seeing the exact same failure when running make test with runtime master tip.

@bergwolf
Copy link
Member

FYI, the root cause is how we flush coverage report in TestMain(). I fixed it in kata-containers/runtime#2371

@jodh-intel
Copy link
Contributor Author

@bergwolf - The code is in master fwics? Good catch on 2371 though!

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.

Specify output directory for "go test" to ensure tests don't fail
5 participants