Combining appengine tests into coverage #287

theacodes · 2015-08-25T17:05:40Z

Adds appengine tests to coverage tox env. The output from tox -e coverage is misleading:

 ~  workspace  oauth2client  master  $  tox -e cover
GLOB sdist-make: /Users/jonwayne/workspace/oauth2client/setup.py
cover inst-nodeps: /Users/jonwayne/workspace/oauth2client/.tox/dist/oauth2client-1.4.12.zip
cover installed: beautifulsoup4==4.4.0,cffi==1.2.1,coverage==3.7.1,cryptography==1.0,Django==1.8.4,enum34==1.0.4,Flask==0.10.1,funcsigs==0.4,httplib2==0.9.1,idna==2.0,ipaddress==1.0.14,itsdangerous==0.24,Jinja2==2.8,keyring==5.4,MarkupSafe==0.23,mock==1.3.0,nose==1.3.7,NoseGAE==0.5.7,oauth2client==1.4.12,pbr==1.6.0,pyasn1==0.1.8,pyasn1-modules==0.0.7,pycparser==2.14,pycrypto==2.6.1,pyOpenSSL==0.15.1,rsa==3.2,six==1.9.0,waitress==0.8.9,WebOb==1.4.1,WebTest==2.0.18,Werkzeug==0.10.4,wheel==0.24.0
cover runtests: PYTHONHASHSEED='4171150552'
cover runtests: commands[0] | nosetests --with-coverage --cover-package=oauth2client --cover-erase --cover-tests --cover-branches --ignore-files=test_appengine\.py
.............................................................................................................................................................................................................................................................................
Name                           Stmts   Miss Branch BrMiss  Cover   Missing
--------------------------------------------------------------------------
oauth2client                       7      0      0      0   100%
oauth2client._helpers             27      0      6      0   100%
oauth2client._openssl_crypt       41      0      4      0   100%
oauth2client._pycrypto_crypt      40      0      4      0   100%
oauth2client.client              725     83    207     44    86%   57-58, 164, 206, 215, 224, 232, 292-296, 355, 365, 372, 724, 741, 776, 889, 892-894, 931-932, 1221, 1237, 1258, 1345, 1418-1422, 1489-1491, 1495-1497, 1551, 1572, 1701, 1875-1896, 1999-2004, 2007, 2017, 2029-2067, 2093, 2095, 2098, 2113, 2222, 2227-2233
oauth2client.clientsecrets        49      0     18      0   100%
oauth2client.crypt                87     14     28      6    83%   44-49, 54-56, 62-66, 153, 167
oauth2client.devshell             53      0      8      0   100%
oauth2client.django_orm           68     26     22     14    56%   46, 48, 53, 71, 73, 78, 101-104, 112-120, 131-140, 145-146
oauth2client.file                 47      0      4      0   100%
oauth2client.flask_util          167      0     40      0   100%
oauth2client.gce                  39      0      4      0   100%
oauth2client.keyring_storage      29      0      2      0   100%
oauth2client.locked_file         183    101     64     47    40%   91, 100, 104, 124-158, 162-169, 173, 196, 216-230, 239-240, 245-329, 351, 356
oauth2client.multistore_file     165     17     24      7    87%   287-294, 360-363, 368-369, 372, 379-380, 386-388, 466-467
oauth2client.service_account      54      0      0      0   100%
oauth2client.tools               109     65     18     16    36%   54-55, 165-239, 244, 249, 252
oauth2client.util                 55     12     18      7    74%   138-141, 148-149, 219-226
oauth2client.xsrfutil             43      0     10      0   100%
--------------------------------------------------------------------------
TOTAL                           1988    318    481    141    81%
----------------------------------------------------------------------
Ran 269 tests in 8.911s

OK
cover runtests: commands[1] | nosetests --with-coverage --cover-package=oauth2client --with-gae --cover-tests --cover-branches --gae-application=tests/data --gae-lib-root=/Users/jonwayne/Downloads/google_appengine --logging-level=INFO tests/test_appengine.py
...................................
Name                           Stmts   Miss Branch BrMiss  Cover   Missing
--------------------------------------------------------------------------
oauth2client                       7      0      0      0   100%
oauth2client._helpers             27      0      6      0   100%
oauth2client._openssl_crypt       41      0      4      0   100%
oauth2client._pycrypto_crypt      40      0      4      0   100%
oauth2client.appengine           337     29     87     20    88%   53-54, 233, 238, 271, 301, 303, 306-307, 314, 347, 362, 378, 382-383, 413, 437, 716-718, 733-734, 758, 800-801, 866, 929, 977, 1016
oauth2client.client              725     82    207     43    87%   57-58, 164, 206, 215, 224, 232, 292-296, 355, 365, 372, 724, 776, 889, 892-894, 931-932, 1221, 1237, 1258, 1345, 1418-1422, 1489-1491, 1495-1497, 1551, 1572, 1701, 1875-1896, 1999-2004, 2007, 2017, 2029-2067, 2093, 2095, 2098, 2113, 2222, 2227-2233
oauth2client.clientsecrets        49      0     18      0   100%
oauth2client.crypt                87     14     28      6    83%   44-49, 54-56, 62-66, 153, 167
oauth2client.util                 55      6     18      6    84%   138-141, 148-149, 220
oauth2client.xsrfutil             43      0     10      0   100%
--------------------------------------------------------------------------
TOTAL                           1411    131    382     75    89%
----------------------------------------------------------------------
Ran 35 tests in 0.373s

OK
__________________________________________________________________________ summary ___________________________________________________________________________
  cover: commands succeeded
  congratulations :)

Running coverage report shows that the coverage is actually combined between the two runs:

Name                           Stmts   Miss Branch BrMiss  Cover
----------------------------------------------------------------
oauth2client/__init__              7      0      0      0   100%
oauth2client/_helpers             27      0      6      0   100%
oauth2client/_openssl_crypt       41      0      4      0   100%
oauth2client/_pycrypto_crypt      40      0      4      0   100%
oauth2client/appengine           337     29     87     20    88%
oauth2client/client              725     82    207     43    87%
oauth2client/clientsecrets        49      0     18      0   100%
oauth2client/crypt                87     14     28      6    83%
oauth2client/devshell             53      0      8      0   100%
oauth2client/django_orm           68     26     22     14    56%
oauth2client/file                 47      0      4      0   100%
oauth2client/flask_util          167      0     40      0   100%
oauth2client/gce                  39      0      4      0   100%
oauth2client/keyring_storage      29      0      2      0   100%
oauth2client/locked_file         183    101     64     47    40%
oauth2client/multistore_file     165     17     24      7    87%
oauth2client/old_run              77     70     16     16     8%
oauth2client/service_account      54      0      0      0   100%
oauth2client/tools               109     65     18     16    36%
oauth2client/util                 55      6     18      6    84%
oauth2client/xsrfutil             43      0     10      0   100%
----------------------------------------------------------------
TOTAL                           2402    410    584    175    80%

Coveralls should pick up the combined coverage.

This removes nosexcover as well.

tox.ini

@@ -56,7 +72,7 @@ basepython = python2.7
 deps = {[testenv]basedeps}
       nosegae
 commands =
-    nosetests --with-gae --gae-application=tests/data --logging-level=INFO {posargs} tests/test_appengine.py
+    nosetests --with-gae --gae-application=tests/data --logging-level=INFO tests/test_appengine.py


dhermes · 2015-08-25T17:16:59Z

@jonparrott Can we ~~surprise~~ suppress the twice run output and just use coverage report? Is there a way to get missing line numbers in coverage report?

dhermes · 2015-08-25T17:18:31Z

I want to hold off on removing xcover until we understand it better (e.g. maybe Tres will have something to say on the issue).

theacodes · 2015-08-25T17:22:36Z

There doesn't seem to be a way to suppress the immediate report on each run. As mentioned, I can add coverage report at the end.

theacodes · 2015-08-25T17:23:24Z

Yes, coverage report -m will show missing numbers:

Name                           Stmts   Miss Branch BrMiss  Cover   Missing
--------------------------------------------------------------------------
oauth2client/__init__              7      0      0      0   100%
oauth2client/_helpers             27      0      6      0   100%
oauth2client/_openssl_crypt       41      0      4      0   100%
oauth2client/_pycrypto_crypt      40      0      4      0   100%
oauth2client/appengine           337     29     87     20    88%   53-54, 233, 238, 271, 301, 303, 306-307, 314, 347, 362, 378, 382-383, 413, 437, 716-718, 733-734, 758, 800-801, 866, 929, 977, 1016
oauth2client/client              725     82    207     43    87%   57-58, 164, 206, 215, 224, 232, 292-296, 355, 365, 372, 724, 776, 889, 892-894, 931-932, 1221, 1237, 1258, 1345, 1418-1422, 1489-1491, 1495-1497, 1551, 1572, 1701, 1875-1896, 1999-2004, 2007, 2017, 2029-2067, 2093, 2095, 2098, 2113, 2222, 2227-2233
oauth2client/clientsecrets        49      0     18      0   100%
oauth2client/crypt                87     14     28      6    83%   44-49, 54-56, 62-66, 153, 167
oauth2client/devshell             53      0      8      0   100%
oauth2client/django_orm           68     26     22     14    56%   46, 48, 53, 71, 73, 78, 101-104, 112-120, 131-140, 145-146
oauth2client/file                 47      0      4      0   100%
oauth2client/flask_util          167      0     40      0   100%
oauth2client/gce                  39      0      4      0   100%
oauth2client/keyring_storage      29      0      2      0   100%
oauth2client/locked_file         183    101     64     47    40%   91, 100, 104, 124-158, 162-169, 173, 196, 216-230, 239-240, 245-329, 351, 356
oauth2client/multistore_file     165     17     24      7    87%   287-294, 360-363, 368-369, 372, 379-380, 386-388, 466-467
oauth2client/old_run              77     70     16     16     8%   26-164
oauth2client/service_account      54      0      0      0   100%
oauth2client/tools               109     65     18     16    36%   54-55, 165-239, 244, 249, 252
oauth2client/util                 55      6     18      6    84%   138-141, 148-149, 220
oauth2client/xsrfutil             43      0     10      0   100%
--------------------------------------------------------------------------
TOTAL                           2402    410    584    175    80%

dhermes · 2015-08-25T17:32:01Z

Yay for coverage report -m.

As for suppressing output, we could do it the old-fashioned way: nosetests ... > /dev/null 2>&1.

theacodes · 2015-08-25T17:32:51Z

lol... uh. Yes, I suppose we could do that, but I'd really rather not. :P

dhermes · 2015-08-25T17:36:32Z

Another option is to redirect the report to HTML:

http://stackoverflow.com/questions/5065455/how-to-disable-nose-tests-coverage-report
http://stackoverflow.com/questions/11725193/disabling-nose-coverage-report-to-stdout-when-html-report-is-enabled

Though I'm not sure that will leave around the bits in .coverage which coverage report picks up.

theacodes · 2015-08-25T17:44:10Z

Meh, that also feels really hacky.

theacodes · 2015-08-25T17:45:28Z

Pushed a new commit. This adds a base cover environment in tox, and modifies cover and coveralls to use that, plus now cover will run coverage report -m --fail-under 80.

tox.ini

    pypy: with_gmp=no
 commands = nosetests --ignore-files=test_appengine\.py {posargs}

-[testenv:cover]
+[coverbase]


dhermes · 2015-08-25T17:54:31Z

Meh, that also feels really hacky.

We have a trade-off here between what feels hacky and what the output looks like. I'm valuing the output over the stuff we have to do.

Something else we haven't mentioned: Why is --ignore-files=test_appengine\.py even necessary?

When I removed it (and had the google_appengine SDK in the PYTHONPATH) the issue was caused by

tests/test_django_orm.py:32:    use_library('django', '1.5')

theacodes · 2015-08-25T17:57:17Z

I'll look into suppressing the output.

Something else we haven't mentioned: Why is --ignore-files=test_appengine.py even necessary?

If we add an import check for dev_appserver or google.appengine and raise skiptest we no longer have to explicitly exclude it. However, we do still need to run the test itself separately.

dhermes · 2015-08-25T18:00:06Z

We don't want to skip the tests. We want to check the coverage. I don't think the conflict needs to occur, but it may take a little work to get them to all run together.

theacodes · 2015-08-25T18:02:11Z

I'm not sure what you mean?

dhermes · 2015-08-25T18:13:09Z

If we add an import check for dev_appserver or google.appengine and raise skiptest

We don't want raise SkipTest because we want to get a complete picture of coverage.

theacodes · 2015-08-25T18:14:20Z

I'm still not following. Are you wanting to move to one run of nosetests instead of two?

dhermes · 2015-08-25T18:20:24Z

Yes that was the point of

Why is --ignore-files=test_appengine\.py even necessary?

theacodes · 2015-08-25T18:22:53Z

I've been down that path, and it's not a fun one. dev_appsever / nosegae does a lot of horrible, horrible things to python. It's almost impossible to undo what it does enough to be certain that your other tests aren't tainted by GAE.

dhermes · 2015-08-25T18:26:44Z

OK I trust you. Then let's focus on the output suppression?

Also, any idea why

oauth2client
oauth2client._helpers
oauth2client._openssl_crypt
oauth2client._pycrypto_crypt
oauth2client.appengine
oauth2client.client
oauth2client.clientsecrets
oauth2client.crypt
oauth2client.util
oauth2client.xsrfutil

all show up when just oauth2client.appengine is tested?

theacodes · 2015-08-25T18:29:58Z

all show up when just oauth2client.appengine is tested?

Probably because those modules are used by the appengine module.

dhermes · 2015-08-25T18:50:12Z

I think having to run --gae-lib-root={env:GAE_PYTHONPATH} could be problematic. Do we have Travis set-up for this? (I just did an wget https://patch-diff.githubusercontent.com/raw/google/oauth2client/pull/287.patch, then git am 287.patch so I could play around with this.)

I feel like the commands are so long, they belong in a bash script anyhow. (How does that impact Windows users? Or even zsh users?)

theacodes · 2015-08-25T18:52:54Z

Travis will work for GAE_PYTHONPATH, as it's set for the app engine tests.

I feel like the commands are so long, they belong in a bash script anyhow. (How does that impact Windows users? Or even zsh users?)

That is not fun. :P It's not unheard of for your nose commands to be a bit long for coverage.

dhermes · 2015-08-25T18:54:06Z

FYI I figured out how to limit the output of just the appengine coverage report:

diff --git a/tox.ini b/tox.ini
index 0cfbac7..5e05741 100644
--- a/tox.ini
+++ b/tox.ini
@@ -28,7 +28,7 @@ commands =
       --ignore-files=test_appengine\.py
     nosetests \
       --with-coverage \
-      --cover-package=oauth2client \
+      --cover-package=oauth2client.appengine \
       --with-gae \
       --cover-tests \
       --cover-branches \

dhermes · 2015-08-25T18:55:43Z

I had forgotten that you set

.travis.yml:16:  - GAE_PYTHONPATH=${HOME}/.cache/google_appengine

before.

That can keep it from blocking this PR, but I still plan on making it smarter.

theacodes · 2015-08-25T19:32:03Z

Alright, I pushed that diff you sent over, so the output it now just the appengine module.

Do you still want to silence the output?

dhermes · 2015-08-25T19:34:00Z

Let's just leave it as-is for now and deal with it if it becomes annoying.

I still want to hold off until we hear from Tres about nosexcover.

theacodes · 2015-08-25T19:35:47Z

SGTM.

Combining appengine tests into coverage

googlebot added the cla: yes label Aug 25, 2015

dhermes reviewed Aug 25, 2015
View reviewed changes

theacodes force-pushed the master branch from 5c79037 to 606bcc9 Compare August 25, 2015 17:44

dhermes reviewed Aug 25, 2015
View reviewed changes

tox.ini

pypy: with_gmp=no

commands = nosetests --ignore-files=test_appengine\.py {posargs}

[testenv:cover]

[coverbase]

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

theacodes force-pushed the master branch from 606bcc9 to 7e4f64c Compare August 25, 2015 17:52

Combining appengine tests into coverage

a4bd21a

theacodes force-pushed the master branch from 7e4f64c to a4bd21a Compare August 25, 2015 19:31

dhermes mentioned this pull request Aug 25, 2015

Incorporate test_appengine.py into coverage report #286

Closed

dhermes added a commit that referenced this pull request Aug 25, 2015

Merge pull request #287 from jonparrott/master

f29c164

Combining appengine tests into coverage

dhermes merged commit f29c164 into googleapis:master Aug 25, 2015

dhermes mentioned this pull request Aug 26, 2015

Releasing 1.5.0. #292

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combining appengine tests into coverage #287

Combining appengine tests into coverage #287

theacodes commented Aug 25, 2015

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

theacodes commented Aug 25, 2015

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

Combining appengine tests into coverage #287

Combining appengine tests into coverage #287

Conversation

theacodes commented Aug 25, 2015

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

theacodes commented Aug 25, 2015

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015

dhermes commented Aug 25, 2015

theacodes commented Aug 25, 2015