Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes test for running code coverage #245

Merged
merged 2 commits into from Aug 13, 2017
Merged

Fixes test for running code coverage #245

merged 2 commits into from Aug 13, 2017

Conversation

hlapp
Copy link
Member

@hlapp hlapp commented Aug 13, 2017

Apparently COVERAGE gets set (to a non-empty value) even if not defined as COVERAGE=1 in the build environment.

I don't know where the underlying script (coverage-setup) is to verify, but I hope it doesn't get set to something other than 0. @zmughal can you check?

Apparently COVERAGE gets set even if not defined as `COVERAGE=1` in the build
environment.
@zmughal
Copy link
Member

zmughal commented Aug 13, 2017

It does get set to something other than zero at some point, but all values other than zero indicate that we are running code coverage (e.g., if COVERAGE=coveralls).

[ -z "$COVERAGE"        ] && export COVERAGE=0
[ "$COVERAGE" == "1" ] && export COVERAGE=coveralls # drop this eventually

@@ -25,7 +25,7 @@ if [[ -z "$DOCKERHUB_TOKEN" ]] ; then
exit 1
fi

if [[ -n "$COVERAGE" ]] ; then
if [[ -n "$COVERAGE" && $COVERAGE -ne 0 ]] ; then
Copy link
Member

@zmughal zmughal Aug 13, 2017

Choose a reason for hiding this comment

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

This condition matches the one here:

 if [ -n "$COVERAGE" ] && [ "$COVERAGE" != "0" ]; then

so this should be correct. It might be better to use the string comparison (!=) rather than the integer one (-ne).

Copy link
Member Author

Choose a reason for hiding this comment

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

So by default it gets set to 0, and if $COVERAGE is already set to 1, it gets changed to "coveralls".

This means the test should be good, but I'm happy to change that to a string comparison, it shouldn't make any difference.

@hlapp hlapp merged commit dfb758f into master Aug 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants