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

Set location of coverage file in Travis #2937

Merged
merged 2 commits into from
Feb 1, 2019

Conversation

kinow
Copy link
Member

@kinow kinow commented Jan 31, 2019

Adding a sed command to our .travis.yml to use an absolute directory for the coverage data file. This way when processes are forked, they will use the same location, instead of a location relative to their working directory.

This should increase the coverage of the project, and cover files such as daemonize.py.

Bruno

@kinow kinow self-assigned this Jan 31, 2019
@kinow kinow added the small label Jan 31, 2019
@kinow kinow added this to the soon milestone Jan 31, 2019
@kinow kinow force-pushed the set-location-of-coverage-file branch from 350a548 to 7088d04 Compare January 31, 2019 05:36
@kinow kinow force-pushed the set-location-of-coverage-file branch from 7088d04 to 0534de0 Compare January 31, 2019 06:12
@kinow
Copy link
Member Author

kinow commented Jan 31, 2019

It looks like the daemonize.py is still not covered in the coverage report for this branch/PR 😭

@oliver-sanders
Copy link
Member

Dammit!

@kinow kinow force-pushed the set-location-of-coverage-file branch from be922aa to 0534de0 Compare January 31, 2019 21:59
@kinow
Copy link
Member Author

kinow commented Feb 1, 2019

😠 tried moving the .coveragerc under /tmp/ and then specify it with coverage run --rcfile... but I guess there are still relative folders... coverage report still giving 0 runs for the daemonize.py file

@kinow
Copy link
Member Author

kinow commented Feb 1, 2019

@oliver-sanders and @matthewrmshin,

I followed my steps from yesterday again this afternoon. And succeeded in running a local coverage command that included scheduler.py and daemonize.py.

But then I realized there is something funny still happening when it runs with Travis.

I've succeeded in getting the coverage for those files, when I ran from command line something similar to:

$ export TRAVIS_BUILD_DIR=$(pwd -P)
$ sed -i -e "s|data_file=.coverage|data_file=${TRAVIS_BUILD_DIR}/.coverage|g" .coveragerc
$ export COVERAGE_PROCESS_START="${TRAVIS_BUILD_DIR}/.coveragerc"
$ export PYTHONPATH=$(pwd -P)/.travis
$ cylc register upstream tests/suite-state/upstream
$ cylc register polling tests/suite-state/polling
$ cylc run upstream
$ cylc run --no-detach polling --set UPSTREAM=upstream
$ coverage combine --append
$ coverage html

Note that I don't even need to run coverage run. It produces the right values in the reports.

However, when instead of registering the suites and running them, I call the test-battery command instead, it doesn't produce the right values:

$ export TRAVIS_BUILD_DIR=$(pwd -P)
$ sed -i -e "s|data_file=.coverage|data_file=${TRAVIS_BUILD_DIR}/.coverage|g" .coveragerc
$ export COVERAGE_PROCESS_START="${TRAVIS_BUILD_DIR}/.coveragerc"
$ export PYTHONPATH=$(pwd -P)/.travis
$ cylc test-battery tests/suite-state/01-polling.t
$ coverage combine --append
$ coverage html

If I call prove, it also doesn't work.

$ export TRAVIS_BUILD_DIR=$(pwd -P)
$ sed -i -e "s|data_file=.coverage|data_file=${TRAVIS_BUILD_DIR}/.coverage|g" .coveragerc
$ export COVERAGE_PROCESS_START="${TRAVIS_BUILD_DIR}/.coveragerc"
$ export PYTHONPATH=$(pwd -P)/.travis
$ export CYLC_TEST_RUN_GENERIC=${CYLC_TEST_RUN_GENERIC:-true}
$ export CYLC_TEST_RUN_PLATFORM=${CYLC_TEST_RUN_PLATFORM:-true}
$ export CYLC_TEST_SKIP=${CYLC_TEST_SKIP:-}
$ export CYLC_TEST_IS_GENERIC=true
$ export CYLC_TEST_TIME_INIT="$(date -u +'%Y%m%dT%H%M%SZ')"
$ prove -s -r "./tests/suite-state/01-polling.t"
$ coverage combine --append
$ coverage html

Finally, it does work if I alter 01-polling.t, and instead of suite_run_ok $TEST_NAME cylc run --reference-test --debug --no-detach --set UPSTREAM=$UPSTREAM $SUITE_NAME, I remove the suite_run_ok $TEST_NAME part. So the following works.

$ export TRAVIS_BUILD_DIR=$(pwd -P)
$ sed -i -e "s|data_file=.coverage|data_file=${TRAVIS_BUILD_DIR}/.coverage|g" .coveragerc
$ export COVERAGE_PROCESS_START="${TRAVIS_BUILD_DIR}/.coveragerc"
$ export PYTHONPATH=$(pwd -P)/.travis
$ export CYLC_TEST_RUN_GENERIC=${CYLC_TEST_RUN_GENERIC:-true}
$ export CYLC_TEST_RUN_PLATFORM=${CYLC_TEST_RUN_PLATFORM:-true}
$ export CYLC_TEST_SKIP=${CYLC_TEST_SKIP:-}
$ export CYLC_TEST_IS_GENERIC=true
$ export CYLC_TEST_TIME_INIT="$(date -u +'%Y%m%dT%H%M%SZ')"
# EDIT 01-polling, leaving just cylc run --reference-test --debug --no-detach --set UPSTREAM=$UPSTREAM $SUITE_NAME
$ prove -s -r "./tests/suite-state/01-polling.t"
$ coverage combine --append
$ coverage html

The test fails due to the number of tests, but still produces the right coverage.

I had a look at the function in test-headers... but couldn't find anything that would initiate a shell without the environment variables necessary for coverage (even altered it a bit to print env | sort, and the variables were there). (EDIT: funny, it worked in NIWA, but just tried all examples at home, and this last one also failed to report the right coverage).

So really awkward that running through cylc test-battery it simply doesn't follow what is in .coveragerc file 😞

@kinow kinow force-pushed the set-location-of-coverage-file branch 2 times, most recently from cd3687f to 8f0e114 Compare February 1, 2019 08:03
@kinow kinow force-pushed the set-location-of-coverage-file branch from 8f0e114 to a314f13 Compare February 1, 2019 08:09
@kinow
Copy link
Member Author

kinow commented Feb 1, 2019

Ra! Enabled the trace option for debugging. Turns out the source directories were not matching. So when the process is forked, it cannot find the coverage RC file, but also it tries to load the source folders (./lib/ and ./bin/) relative to the working directory that may not match Cylc's git checked out directory).

Fixed by adding two more sed expressions. It looks like it resolves all relative folders to absolute folders in the configuration file. So hopefully no more surprises when running tests in Travis CI.

And, good news! Coverage went up to 83.99%! 🎉 Might have another look at what is not being tested and see if there's anything to keep an eye on during the Python3 migration. But the coverage of the existing functional tests is really impressive. Well done!! 👏 👏

This pull request is ready for review.

@matthewrmshin
Copy link
Contributor

We are not so bad after all!

@hjoliver
Copy link
Member

hjoliver commented Feb 1, 2019

Brilliant news, made my day 😁 👏

@hjoliver
Copy link
Member

hjoliver commented Feb 1, 2019

(I was a little surprised that our tests didn't give better coverage than seemed to be the case at first... so nice to find our intuition wasn't too far off after all!)

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

84% seems a bit more realistic to me!

@oliver-sanders oliver-sanders merged commit 71480d8 into cylc:master Feb 1, 2019
@matthewrmshin matthewrmshin modified the milestones: soon, next-release Feb 1, 2019
@matthewrmshin matthewrmshin mentioned this pull request Feb 8, 2019
@kinow kinow deleted the set-location-of-coverage-file branch March 14, 2019 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants