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

Add coverage to the project reports. #2766

Merged
merged 19 commits into from
Nov 23, 2018

Conversation

kinow
Copy link
Member

@kinow kinow commented Aug 31, 2018

Travis-CI configuration was updated to use build stages. The build matrix was moved to the test stage, and extra configuration for coverage.py and codacy were added.

The tool used to collect coverage from the tests in Coverage.py, which is supported by multiple build and reporting systems.

Due to the nature of the tests in cylc, and to how they run in parallel in different containers in Travis-CI, we had to use a sitecustomize.py script.

As Codacy was already used for code analysis, this pull request includes settings to export the coverage report to Codacy. I assume the Codacy token is already set up. So maybe just merging the pull request would already make the coverage report available in the cylc dashboard.

It is also possible to get the coverage of pull requests.

Cheers
Bruno

@kinow
Copy link
Member Author

kinow commented Aug 31, 2018

The branch from where this pull request is coming from has coverage already in Codacy. The Travis-CI build can be seen here: https://travis-ci.org/kinow/cylc

And the Codacy report is also available here: https://app.codacy.com/project/kinow/cylc/dashboard?branchId=8624609

And here are some screenshots, showing the current coverage of 34%:

screenshot_2018-08-31_00-37-31

screenshot_2018-08-31_17-26-17

screenshot_2018-08-31_17-26-43

screenshot_2018-08-31_17-34-54

screenshot_2018-08-31_17-35-13

@kinow
Copy link
Member Author

kinow commented Aug 31, 2018

Codacy is unhappy with the use of subprocess in the cover.py script. That script is used to invoke the tests. Previously, the test command was in .travis-ci.yml. But now we have to start it with coverage run, which only supports Python scripts.

So that script is just a Python wrapper, so that coverage run works normally.

@kinow
Copy link
Member Author

kinow commented Aug 31, 2018

Added a commit with the coverage badge, and mentioned the issue #2765 too.

I believe someone with karma will have to set up the Codacy token in order for the coverage to work in Codacy.

The first build in Travis for this pull request tried to upload the coverage to the cylc/cylc Codacy dashboard, but it failed with

08/31 06:08:11 �[1;31mERROR�[0;39m �[36mc.c.rules.ConfigurationRules:22�[0;39m - Invalid configuration: Project token not provided and not available in environment variable "CODACY_PROJECT_TOKEN" 

At least Travis-CI build is still working :-)

@hjoliver
Copy link
Member

hjoliver commented Sep 2, 2018

@cylc/core - note you can drill down (from the page shown in the last of Bruno's screenshots above) to see exactly what lines of code are touched by the test battery. Coverage does not look as good as I had expected/hoped. Hopefully our heavy use of full regression tests means that code associated with "normal" Cylc usage is well covered, but maybe we are missing a lot of edge cases (which may be difficult to target with regression tests in a vanilla test environment).

So perhaps this warrants a change to our testing strategy? How about, for all new features and fixes:

  • write new (or modify existing) regression tests to support major functionality
  • guided by coverage data, write unit tests (where possible) to nail any code that is not touched by the regression tests.
  • (and we can incrementally retro-fit more unit tests to existing files touched by new PRs)

Perhaps we should consider full coverage by unit tests (where possible) but my feeling is that would be a lot of redundant effort given that we have to have regression tests as well, to ensure that the functionality actually relied on by users works.

@kinow might have opinions on this?

@hjoliver hjoliver self-requested a review September 2, 2018 20:26
@hjoliver
Copy link
Member

hjoliver commented Sep 2, 2018

Pinged @oliver-sanders as first reviewer, since this remodels his recent "chunking" of the test battery (if he has time pre-Brussels).

@kinow
Copy link
Member Author

kinow commented Sep 2, 2018

@hjoliver ,

Coverage does not look as good as I had expected/hoped. Hopefully our heavy use of full regression tests means that code associated with "normal" Cylc usage is well covered, but maybe we are missing a lot of edge cases (which may be difficult to target with regression tests in a vanilla test environment).

That's my thinking as well. I want to keep running the coverage locally, experimenting with the settings, and confirming that it is not missing any tests when reporting the coverage.

There are many cases - especially exception handling - where I think it could be easier to be covered by unit tests (and some times simple mocks). Not necessarily to increase the coverage, but more to make sure the logic behind the exception handling is working as expected. Same for validation.

I haven't checked, but if I remember well, we have some files in Cylc's folder with both the main code, and the unit test. Might be worth checking if these are not interfering with the reports (and in that case, it may be a good idea to move the test to its own folder).

Happy to take more reviews/suggestions/comments :-)

Thanks!
Bruno

@hjoliver
Copy link
Member

hjoliver commented Sep 2, 2018

@kinow - for the unit tests that we do have (admittedly not so many, as discussed!) I believe we run them all as special tests within cylc test-battery, so should not be missed in the coverage reports.

@kinow
Copy link
Member Author

kinow commented Sep 2, 2018

@hjoliver, sorry I meant that the reports could be including the test source. Normally coverage tools make a distinction between "source code to be covered", and the "tests".

The source to be covered included lib/cylc, and excluded folders like tests, and the GUI files. But, as we have source code mixed with the tests, I think our coverage number is not so precise.

Here's an example of code that appears as not covered: https://app.codacy.com/app/kinow/cylc/file/21757250719/coverage?bid=8624609&fileBranchId=8624609#l511

But I believe the code above is a test code, and not really part of lib/cylc/graph_parser.py. So if that's indeed the case, I'd say that there's no easy way to fix the coverage report without moving the tests out of the code... I think :) (haven't drunk any coffee today yet)

@hjoliver
Copy link
Member

hjoliver commented Sep 2, 2018

Ah, got it. Yeah, that does seem a good reason to separate unit tests from source.

@kinow
Copy link
Member Author

kinow commented Sep 7, 2018

Running a small experiment, where I've added a commit with the unitest.TestCase instances moved to a different package. Tried to match settings in Travis-CI and Coverage.py commands, in order to get a better coverage number... now wait and see if Travis-CI will run successfully :-)

@kinow
Copy link
Member Author

kinow commented Sep 7, 2018

If I am reading the Codacy user interface correctly, coverage went up to 38%.

image

Now omitting the isodatetime folder too, as I had a look, and it looks like this folder is a copy of the metomi/isodatetime module.

@kinow
Copy link
Member Author

kinow commented Sep 7, 2018

Done! The current Travis-CI job for Cylc doesn't have the Codacy integration enabled, but I have enabled Travis-CI and Codacy for my fork. So the latest coverage report (as well as the coverage trend) can be seen here:

After omitting isodatetime, coverage went up to 42%. I think xdot.py and cylc_xdot.py could be possibly omitted from the reports too, but quite happy with the current setup.

image

Unit tests would be under the lib/cylc/tests folder now, and executed with PyTest. Integration/functional tests would be under the tests/ folder, and keep being executed with Shell/Python/prove. The report that we have is an aggregated report.

There are a few files that have 0% coverage, but contain Python 2 statements that would have to be ported to Python 3. So keen to start writing some unit/functional tests to cover the current behaviour too, and try to reduce the likelihood of regressions (that may help the work on the new Web GUI as well I think).

Bruno

@hjoliver
Copy link
Member

hjoliver commented Sep 9, 2018

Nice. Yeah we should definitely look at those 0% files and write some tests. I'll try to take a detailed look at the report soon.

@hjoliver
Copy link
Member

@kinow - we took a look at this yesterday and we're generally impressed at how useful the coverage reports will be. We might as well merge it and start using it, I think. Is this PR good to go as far as you're concerned, once #2766 (comment) is done?

@kinow
Copy link
Member Author

kinow commented Sep 21, 2018

Hi @hjoliver , I rebased it this week, and had another look at the changes, and so far everything looks OK to me. The tests appear to be working OK in Travis-CI, and the report also appears to be generated correctly.

So +1, if someone can look at the settings for Codacy, then I think this PR is good to go :-) Hopefully this report - as well as the other reports/reviews - will support us during the upcoming changes for web gui, python3 🎉

Thank you

@kinow
Copy link
Member Author

kinow commented Oct 16, 2018

Rebased, and added work done in isodatetime. The job in travis will take a while to finish (unless I did something wrong and it immediately fails).

Assuming everything works with no issues, I will have another look at the changes, then separate and squash some commits for clarity, and it should be ready for review again 👍

@cylc cylc deleted a comment Oct 16, 2018
@cylc cylc deleted a comment Oct 16, 2018
@cylc cylc deleted a comment Oct 16, 2018
@cylc cylc deleted a comment Oct 16, 2018
@oliver-sanders
Copy link
Member

FYI more unittests have snuck in (scheduler_cli)

@hjoliver
Copy link
Member

@kinow - should I be able to see a coverage report now? (can't find it).

@cylc cylc deleted a comment Nov 23, 2018
@cylc cylc deleted a comment Nov 23, 2018
@cylc cylc deleted a comment Nov 23, 2018
@cylc cylc deleted a comment Nov 23, 2018
@cylc cylc deleted a comment Nov 23, 2018
@cylc cylc deleted a comment Nov 23, 2018
@cylc cylc deleted a comment Nov 23, 2018
@kinow
Copy link
Member Author

kinow commented Nov 23, 2018

@hjoliver pointed in #2877 that some unit tests were still executed with the test-battery command. Some were not even being executed anymore, and the unitttest test class had already been moved. So I've removed the tests that pytest was already taking care of.

Also include an extra commit to disable a doctest from parsec. That is already executed by pytest, and included in the coverage reports 👍

@cylc cylc deleted a comment Nov 23, 2018
@kinow
Copy link
Member Author

kinow commented Nov 23, 2018

Replaced deprecated failUnless by assertTrue in the last commit, to make a few warnings go away when running pytest.

@matthewrmshin
Copy link
Contributor

This is now looking good.

In my environment, I need to exclude these for lacking GTK and EmPy:

diff --git a/pytest.ini b/pytest.ini
index 3a8ae0802..c9e260686 100644
--- a/pytest.ini
+++ b/pytest.ini
@@ -17,11 +17,14 @@
 [pytest]
 addopts = --verbose
     --doctest-modules
+    --ignore=lib/parsec/empysupport.py
     --ignore=lib/parsec/tests/getcfg/bin/one-line.py
     --ignore=lib/parsec/tests/synonyms/bin/synonyms.py
     --ignore=lib/parsec/tests/nullcfg/bin/empty.py
     --ignore=lib/parsec/example
     --ignore=lib/cherrypy
+    --ignore=lib/cylc/cylc_xdot.py
+    --ignore=lib/cylc/gui
     --ignore=lib/isodatetime
     --ignore=lib/markupsafe
 testpaths =

I also got some deprecation warnings, because we are using .failUnless in
lib/cylc/tests/test_host_appointer.py.

@kinow
Copy link
Member Author

kinow commented Nov 23, 2018

@matthewrmshin are you still have problems with the test_empysupport.py? I thought my fix would work on your environment too.

And as for the errors with GTK and xdot... what if we add these lines to pytest.ini Matt? These files are not covered by unit tests (as far as I recall). And are also ignored in our coverage report anyway as far as I know.

So that way it would work for everybody with pytest 😬

@kinow
Copy link
Member Author

kinow commented Nov 23, 2018

(Oh, ignore my first comment. I thought it said test_empysupport.)

@matthewrmshin
Copy link
Contributor

Yes, would be useful to ignore the GTK stuffs.

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.

This is good enough to go, let's do it!

There are a few followups which I think need to be looked into:

  • https://pypi.org/project/pytest-parallel/ - later on if our unittests start taking more than a few seconds?
  • Better system for automatically skipping unit-tests based on software dependencies. The current system in the cylc test-battery works well, manually editing the .pytest file isn't great.
  • Bring together the pylint and cylc test-battery commands somehow. For example the following change to cylc test-battery will run pytest and pycodestyle before functional tests unless a test or list of tests is explicitly provided (e.g. by cylc test-battery --chunk):
diff --git a/bin/cylc-test-battery b/bin/cylc-test-battery
index c34dde88e..388363588 100755
--- a/bin/cylc-test-battery
+++ b/bin/cylc-test-battery
@@ -149,6 +149,29 @@ if [[ -w "$CYLC_DIR/lib" ]]; then
     python -mcompileall -q "$CYLC_DIR/lib"
 fi
 
+LINTERS=(pycodestyle pep8)
+
+# Run python tests.
+if [[ ! -n "${@:-}" ]]; then  # TODO: this better (to support other CLI options e.g. -v and -j)
+    # Code style.
+    for linter in "${LINTERS[@]}"; do
+        if which "${linter}" >/dev/null 2>&1; then
+            "${linter}" --ignore=E402,W503,W504 \
+                lib/cylc \
+                lib/Jinja2Filters/*.py \
+                lib/parsec/*.py \
+                $(grep -l '#!.*\<python\>' bin/* | grep -v 'cylc-test-battery')
+            break  # run the first linter present on the system
+        fi
+    done
+
+    # Unittests (including doctests).
+    (cd "${CYLC_DIR}/lib/cylc"; pytest -c)
+fi
+
+# Run functional tests.
 if perl -e 'use Test::Harness 3.00' 2>/dev/null; then
     NPROC=$(cylc get-global-config '--item=process pool size')
     if [[ -z "${NPROC}" ]]; then

@kinow
Copy link
Member Author

kinow commented Nov 23, 2018

@oliver-sanders

https://pypi.org/project/pytest-parallel/ - later on if our unittests start taking more than a few seconds?

+1

Better system for automatically skipping unit-tests based on software dependencies. The current system in the cylc test-battery works well, manually editing the .pytest file isn't great.

Once we have pytest configured and running, we can start using markers.

Bring together the pylint and cylc test-battery commands somehow. For example the following change to cylc test-battery will run pytest and pycodestyle before functional tests unless a test or list of tests is explicitly provided (e.g. by cylc test-battery --chunk):

I prefer to leave the test-battery responsible for running the functional tests with shell + Python for Cylc. And have pytest running the tests. setup.py and tox both interact with pytest, none with the test-battery. If we adopted tox to validate our setup.py against multiple Python versions (e.g. 3.3, 3.5, 3.7, etc), we would have to keep changing the test-battery to accommodate these changes.

But we can discuss that later in Melbourne or in cylc/admin, etc. 👍

@matthewrmshin
Copy link
Contributor

I am going to merge this and raise a further PR to modify pytest.ini so it works at our site.

@oliver-sanders
Copy link
Member

oliver-sanders commented Nov 23, 2018

Fine by me, we can sort out skips later.

@matthewrmshin matthewrmshin merged commit 100d867 into cylc:master Nov 23, 2018
@cylc cylc deleted a comment Nov 23, 2018
@hjoliver hjoliver mentioned this pull request Nov 24, 2018
7 tasks
@kinow kinow deleted the report-test-coverage branch March 14, 2019 01:30
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

5 participants