Merge lp:~mpontillo/maas/code-coverage into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Run code coverage during "make test", and add build targets to generate coverage reports.

Change "make test" to run the coverage report. Add build targets to generate XML output (for Jenkins) or HTML output (for developers) from the coverage data.

[Mike Pontillo (mpontillo)]

review: Abstain

[Mike Pontillo (mpontillo)]

Note: python-coverage is only required if you want to create a human readable (or machine readable) report from the '.coverage' file. In the future, Jenkins should run the target to create the XML report, and expose it through the web interface. (the Cobertura plugin can be used for this.)

[Raphaël Badin (rvb)]

This looks generally good but you're missing a couple of dependencies.

[0]

This is an excerpt of what make test spits out: http://paste.ubuntu.com/10445321/. As you can see, most lines relate to non-MAAS modules. Isn't it possible to not clutter the output like that?

[1]

The HTML report has lines for the test modules themselves. This is a bit weird. I understand that this can be useful in some cases (to detect dead code in tests) but can't we do something about this? (Like put them on a different page or something). What do you think?

[2]

Looks like you're missing a couple of dependencies here.
$ make coverage-report
rm -rf htmlcov
python-coverage html --include 'src/*'
Couldn't find static file 'jquery.hotkeys.js'
make: *** [coverage-report] Error 1

$ sudo apt-get install libjs-jquery-hotkeys
[...]

$ make coverage-report
rm -rf htmlcov
python-coverage html --include 'src/*'
Couldn't find static file 'jquery.isonscreen.js'
make: *** [coverage-report] Error 1

$ sudo apt-get install libjs-jquery-isonscreen

$ make coverage-report
rm -rf htmlcov
python-coverage html --include 'src/*'
Couldn't find static file 'jquery.tablesorter.min.js'
make: *** [coverage-report] Error 1

$ sudo apt-get install libjs-jquery-tablesorter

review: Needs Fixing

[Gavin Panella (allenap)]

A few comments. I think this is really cool, thank you so much for doing it.

I got a little carried away and made some changes of my own to address my own comments. I also changed some things. The principle is the same though. My patch to your branch: http://paste.ubuntu.com/10452649/

So, basically, it's Needs Fixing, but I'd vote Approve if you can stomach my proposed changes. I hope that's okay.

review: Needs Fixing

[Mike Pontillo (mpontillo)]

Raphael, I like including the tests in the report for the reason you mention, but agree it would be nice to hide them, since they get in the way (and give a false impression of coverage percentage, assuming they all run!). I don't really like the sorting in the report; the test directories are sorted and appear inline with the code under test.

My original thought was to create a "coverage" target that did the same thing as the "test" target, but my Makefile skills are rusty, so I couldn't figure out how to do it without duplicating code in the Makefile. Then I considered a couple of use cases:

(1) You might want to run coverage manually, then invoke the 'make' targets to create the report
(2) You might want to run a coverage report even if the tests fail, so that you can see which lines of code were executed.

For those reasons, I didn't make the coverage targets depend on the 'test' target. But I think there's a good argument to be made for doing so, and perhaps we should create helper scripts for running the tests, and invoke *those* from the Makefile, rather than adding individual commands to the Makefile.

Nice catch on the dependencies. I suppose I must have had those packages installed already. I don't know why they aren't dependencies of python-coverage; seems like that was overlooked when that was packaged..?

Gavin, thanks for your patch! I applied and pushed it into the review branch. I'll test out your changes.

[Gavin Panella (allenap)]

With that patch applied, you can do something like:

  rm coverage.data
  bin/test.cluster --with-coverage
  make coverage-report

and get a report for just the cluster. That's another reason not to connect coverage.data to the test target.

Also, if you do:

  make coverage-report

before creating any coverage data, make will exit with an explanation of what you need to do.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~mpontillo/maas/code-coverage into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Get:2 http://security.ubuntu.com trusty-security Release [62.0 kB]
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates Release [62.0 kB]
Get:5 http://security.ubuntu.com trusty-security/main Sources [71.4 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Get:6 http://security.ubuntu.com trusty-security/universe Sources [17.9 kB]
Get:7 http://security.ubuntu.com trusty-security/main amd64 Packages [220 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Get:8 http://security.ubuntu.com trusty-security/universe amd64 Packages [87.7 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [181 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [105 kB]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [446 kB]
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [254 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Fetched 1,509 kB in 2s (556 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools gjs ipython isc-dhcp-common libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make nodejs-legacy npm pep8 postgresql pyflakes python-bson python-bzrlib python-convoy python-coverage python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-iscpy python-jinja2 python-jsonschema python-lockfile python-lxml python-mock python-netaddr python-netifaces python-nose python-oauth pyth...

[Gavin Panella (allenap)]

Sorry, I gave you an error in my patch.

[Mike Pontillo (mpontillo)]

Thanks. I thought I fixed that, but later realized I had edited the output file in bin/. I'll wait for the full test run to complete before I try to land it again...

=== modified file '.bzrignore'
--- .bzrignore	2015-02-17 23:19:09 +0000
+++ .bzrignore	2015-02-27 22:08:16 +0000
@@ -1,7 +1,6 @@
 *.egg
 *.egg-info
 *.log
-./.coverage
 ./.db.lock
 ./.installed.cfg
 ./.noseids
@@ -12,6 +11,9 @@
 ./acceptance/source
 ./bin
 ./build
+./coverage
+./coverage.data
+./coverage.xml
 ./db
 ./develop-eggs
 ./dist
 
=== added file '.coveragerc'
--- .coveragerc	1970-01-01 00:00:00 +0000
+++ .coveragerc	2015-02-27 22:08:16 +0000
@@ -0,0 +1,10 @@
+[run]
+data_file = coverage.data
+
+[report]
+exclude_lines =
+    # Have to re-enable the standard pragma
+    pragma: no cover
+
+    # Don't complain if tests don't hit defensive assertion code:
+    raise NotImplementedError
 
=== modified file 'Makefile'
--- Makefile	2015-02-27 14:29:16 +0000
+++ Makefile	2015-02-27 22:08:16 +0000
@@ -166,8 +166,24 @@
 test: test-scripts-all = $(wildcard bin/test.*)
 # Don't run bin/test.e2e for now; it breaks.
 test: test-scripts = $(filter-out bin/test.e2e,$(test-scripts-all))
+test: export NOSE_WITH_COVERAGE = 1
 test: build
-	echo $(test-scripts) | xargs -n1 env
+	@$(RM) coverage.data
+	@echo $(test-scripts) | xargs --verbose -n1 env
+
+coverage-report: coverage/index.html
+	sensible-browser $< > /dev/null 2>&1 &
+
+coverage.xml: coverage.data
+	python-coverage xml --include 'src/*' -o $@
+
+coverage/index.html: coverage.data
+	@$(RM) -r $(@D)
+	python-coverage html --include 'src/*' -d $(@D)
+
+coverage.data:
+	@$(error Use `$(MAKE) test` to generate coverage data, or invoke a \
+            test script using the `--with-coverage` flag)
 
 lint: lint-py lint-js lint-doc
 
@@ -246,6 +262,8 @@
 	$(RM) docs/api.rst
 	$(RM) -r docs/_autosummary docs/_build
 	$(RM) -r man/.doctrees
+	$(RM) coverage.data coverage.xml
+	$(RM) -r coverage
 
 distclean: clean stop
 	$(RM) -r bin include lib local
@@ -277,6 +295,7 @@
   build
   check
   clean
+  coverage-report
   dbharness
   distclean
   doc
 
=== modified file 'buildout.cfg'
--- buildout.cfg	2015-02-19 15:17:15 +0000
+++ buildout.cfg	2015-02-27 22:08:16 +0000
@@ -92,11 +92,12 @@
   sys.argv[1:1] = [
       "test", "--noinput", "--exclude=provisioningserver",
       "--exclude=maastesting", "--exclude=maascli",
-      "--logging-level=INFO", "--logging-clear-handlers",
+      "--cover-package=maas,maasserver,metadataserver",
       # Reduce the logging level to INFO here as
       # DebuggingLoggerMiddleware logs the content of all the
       # requests at DEBUG level: we don't want this in the
       # tests as it's too verbose.
+      "--logging-level=INFO", "--logging-clear-handlers",
   ]
 scripts = test.region
 extra-paths =
@@ -120,7 +121,10 @@
 entry-points =
   test.cli=nose.core:TestProgram
 initialization =
-  sys.argv[1:1] = ["--where=src/maascli"]
+  sys.argv[1:1] = [
+    "--where=src/maascli",
+    "--cover-package=apiclient,maascli",
+  ]
 extra-paths = ${cli:extra-paths}
 scripts =
   test.cli
@@ -145,7 +149,10 @@
 entry-points =
   test.testing=nose.core:TestProgram
 initialization =
-  sys.argv[1:1] = ["--where=src/maastesting"]
+  sys.argv[1:1] = [
+    "--where=src/maastesting",
+    "--cover-package=maastesting",
+  ]
 extra-paths = ${common:extra-paths}
 scripts =
   test.testing
@@ -181,7 +188,10 @@
   test.cluster=nose.core:TestProgram
 initialization =
   ${common:initialization}
-  sys.argv[1:1] = ["--where=src/provisioningserver"]
+  sys.argv[1:1] = [
+    "--where=src/provisioningserver",
+    "--cover-package=provisioningserver",
+  ]
 extra-paths = ${cluster:extra-paths}
 scripts =
   test.cluster
@@ -194,7 +204,10 @@
   test.config=nose.core:TestProgram
 initialization =
   ${common:initialization}
-  sys.argv[1:1] = ["--where=etc/maas/templates/commissioning-user-data"]
+  sys.argv[1:1] = [
+    "--where=etc/maas/templates/commissioning-user-data",
+    "--cover-package=snippets",
+  ]
 extra-paths = ${common:extra-paths}
 scripts =
   test.config
 
=== modified file 'required-packages/dev'
--- required-packages/dev	2015-02-26 12:31:44 +0000
+++ required-packages/dev	2015-02-27 22:08:16 +0000
@@ -5,12 +5,15 @@
 firefox
 gjs
 ipython
+libjs-jquery
+libjs-jquery-hotkeys
 libjs-yui3-full
 make
 nodejs-legacy
 npm
 pep8
 pyflakes
+python-coverage
 python-extras
 python-fixtures
 python-flake8