Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Get to 100% code coverage #212

Closed
52 tasks done
dhermes opened this issue Jul 2, 2015 · 16 comments
Closed
52 tasks done

Get to 100% code coverage #212

dhermes opened this issue Jul 2, 2015 · 16 comments

Comments

@dhermes
Copy link
Contributor

dhermes commented Jul 2, 2015


@dhermes
Copy link
Contributor Author

dhermes commented Jul 2, 2015

This will "fix" #209 among other things.

Overall, test status of this library is quite bad and the uber-module client.py and it's uber-test-module test_oauth2client.py could benefit from being split apart.

@dhermes
Copy link
Contributor Author

dhermes commented Jul 6, 2015

First order of business, separate out import madness from crypt.py and put OpenSSL and pyCrypto implementations in separate modules.

dhermes added a commit to dhermes/oauth2client that referenced this issue Jul 7, 2015
Until now, code that depended on PyCrypto or OpenSSL was
defined conditionally (e.g. indented) in `crypt.py`. Rather than
grouping all these together, we factor out the library specific
behavior into standalone modules (but make the modules
private / protected).

In addition, added a `_helpers.py` module with common behavior
that was previously defined in multiple places.

Finally, beefed up some test cases so that the three newly added
modules had 100% test coverage.

Towards googleapis#212.
dhermes added a commit to dhermes/oauth2client that referenced this issue Jul 13, 2015
Until now, code that depended on PyCrypto or OpenSSL was
defined conditionally (e.g. indented) in `crypt.py`. Rather than
grouping all these together, we factor out the library specific
behavior into standalone modules (but make the modules
private / protected).

In addition, added a `_helpers.py` module with common behavior
that was previously defined in multiple places.

Finally, beefed up some test cases so that the three newly added
modules had 100% test coverage.

Towards googleapis#212.
@dhermes
Copy link
Contributor Author

dhermes commented Aug 23, 2015

As of right now (6a9308a, after #284 merged), here is the output of tox -e cover

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 9.415s

I haven't yet figured out how to build in oauth2client/appengine.py but will do so soon-ish.

@dhermes
Copy link
Contributor Author

dhermes commented Aug 24, 2015

@jonparrott You previously mentioned that it was possible to collect coverage from multiple runs. I am looking to do this for oauth2client.appengine.

Care to help? Shall I file a bug and we take it up there / have a PR which closes that bug?

@theacodes
Copy link
Contributor

Yep, file a bug. I'll see what I can do.

@dhermes
Copy link
Contributor Author

dhermes commented Aug 26, 2015

As per discussion with @nathanielmanistaatgoogle in #288, need to be sure to:

  • Take a look at test_jwt.py and see if it should be split up (Clean up Service Account support #211 may force this one)
  • Add direct tests for _openssl_crypt.py rather than relying on test_jwt.py to indirectly test it

@dhermes
Copy link
Contributor Author

dhermes commented Aug 31, 2015

After doing #293 I wanted to make sure I hadn't reduced the coverage of client.py, so actually went through all the lines missing coverage

57-58  # PRAGMA
169  'def _abstract', easy to test
211, 220, 229, 237 -- all abstract methods
297-301 -- import error for old code
360, 370, 377 -- all abstract methods
729 -- branch in method
746 -- branch (default arg)
781 -- branch in method
894 -- error_description in payload (branch)
897 -- store is not None (branch)
898-899 -- exception occurs
936-937 -- error in payload (branch)
1222 -- fall-through to GAE environ for ADC
1238 -- fall-through to GCE environ for ADC
1259 -- fall-through to well-known file for ADC
1346 -- from_stream failure
1419-1423 -- windows keyerror in _get_well_known_file()
1490-1492 -- _get_application_default_credential_GAE() method (untested)
1496-1498 -- _get_application_default_credential_GCE() method (untested)
1552 -- abstract method
1573 -- _RequireCryptoOrDie() failure
1702 -- verify_id_token default input for http
1876-1897 -- DeviceFlowInfo.FromResponse (entire method)
2000-2005 -- branch for non-None value (a log statement and a setter)
2008 -- exception if None value encountered
2018 -- login_hint is set (branch)
2030-2068 -- OAuth2WebServerFlow.step1_get_device_and_user_codes (entire method)
2094 -- branch (two null inputs, need exactly one to be non-null)
2096 -- branch (same as 2096, but two non-null inputs)
2099 -- branch (code input var is None)
2114 -- branch (device_flow_info input var is None)
2223 -- branch (device_uri input var is None)
2228-2234 -- exceptions at end of method

Many of them are abstract methods that are easy to test (i.e. self.assertRaises(NotImplementedError, ...)) and be on your merry way.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 5, 2015

@waprin I just updated the list after #332 got in and noticed oauth2client.contrib.django_util.apps slipped in.

IIRC we discussed during that PR that you might knock off coverage for django_orm as well.

Still down to help got to 100% coverage?

@waprin
Copy link
Contributor

waprin commented Dec 5, 2015

Yes sure. I thinks the apps.py can be tested since we're no longer running Django tests for 2.6 I will either cover django_orm or just rewrite as my next task.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 5, 2015

👍 on the rewrite

@dhermes
Copy link
Contributor Author

dhermes commented Dec 5, 2015

Might want to work upstream from #353 to avoid merge headaches

@dhermes
Copy link
Contributor Author

dhermes commented Feb 23, 2016

@waprin We had previously discussed you getting django_orm.py both 100% covered and maybe even making the code better? Are you still up for that?

UPDATE: While we're at it, oauth2client.contrib.django_util.apps is 0% covered. Any idea why? Should we just ignore the module in our .coveragerc?

@waprin
Copy link
Contributor

waprin commented Mar 14, 2016

I believe apps is not covered because it's only part of later versions of Django that don't support 2.6. Now given coverage runs under Python 2.7 I forget the specific reason for why it's not covered but it was something along those lines, I'd have to revisit.

@dhermes
Copy link
Contributor Author

dhermes commented Mar 14, 2016

tox -e cover only runs with 2.7.

@waprin
Copy link
Contributor

waprin commented Mar 14, 2016

Well there is a pragma nocover in there so it's basically 0% of 0 (well, except for not testing import sys). I think I just needed a Python 2.7+ only test-case, since it's basically a metadata class it didn't seem super-important to test but I'll take another look at it.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 9, 2016

I consider this a successful week!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants