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

Adding tests to coverage report. #297

Merged

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Sep 3, 2015

This reveals there are dead lines in many of our tests. Some are innocuous, like

https://github.com/google/oauth2client/blob/f15e80ee5a4ff07803113204f6b8eda9be65f758/tests/test_xsrfutil.py#L296
https://github.com/google/oauth2client/blob/f15e80ee5a4ff07803113204f6b8eda9be65f758/tests/test_tools.py#L31

while others are caused by using self.fail() statements which never occur:

https://github.com/google/oauth2client/blob/f15e80ee5a4ff07803113204f6b8eda9be65f758/tests/test_service_account.py#L66
https://github.com/google/oauth2client/blob/f15e80ee5a4ff07803113204f6b8eda9be65f758/tests/test_service_account.py#L72


At HEAD right now, tox -e cover gets line coverage on

oauth2client/_helpers
oauth2client/_openssl_crypt
oauth2client/_pycrypto_crypt
oauth2client/appengine
oauth2client/client
oauth2client/clientsecrets
oauth2client/crypt
oauth2client/devshell
oauth2client/django_orm
oauth2client/file
oauth2client/flask_util
oauth2client/gce
oauth2client/keyring_storage
oauth2client/locked_file
oauth2client/multistore_file
oauth2client/service_account
oauth2client/tools
oauth2client/util
oauth2client/xsrfutil

With this PR, the following files also get line coverage (this is the point of --cover-tests BTW):

tests/__init__
tests/http_mock
tests/test__helpers
tests/test__pycrypto_crypt
tests/test_appengine
tests/test_client
tests/test_clientsecrets
tests/test_crypt
tests/test_devshell
tests/test_django_orm
tests/test_file
tests/test_flask_util
tests/test_gce
tests/test_jwt
tests/test_keyring_storage
tests/test_service_account
tests/test_tools
tests/test_util
tests/test_xsrfutil

@dhermes
Copy link
Contributor Author

dhermes commented Sep 3, 2015

I wrote a little script to make links to all the uncovered lines in tests:

tests/http_mock: Line(s) 35 (YAGNI test behavior)
tests/http_mock: Line(s) 37-39 (YAGNI test behavior)
tests/http_mock: Line(s) 111 (YAGNI content == 'echo_request_headers_as_json')
tests/http_mock: Line(s) 116 (YAGNI content == 'echo_request_uri')
tests/http_mock: Line(s) 118 (TypeError that never occurs)
tests/test_appengine: Line(s) 93 (CacheMock.set never gets called)
tests/test_appengine: Line(s) 896 (unittest.main() at end of file)
tests/test_client: Line(s) 125 (CacheMock.set never gets called)
tests/test_client: Line(s) 140 (MockResponse never gets constructed, Danny forgot to delete)
tests/test_client: Line(s) 143-150 (MockResponse never gets constructed, Danny forgot to delete)
tests/test_client: Line(s) 190 (GoogleCredentialsTests.reset_env branch miss)
tests/test_client: Line(s) 341 (Use of self.fail())
tests/test_client: Line(s) 446 (Use of self.fail())
tests/test_client: Line(s) 461 (Use of self.fail())
tests/test_client: Line(s) 481 (Use of self.fail())
tests/test_client: Line(s) 496 (Use of self.fail())
tests/test_client: Line(s) 536 (Use of self.fail())
tests/test_client: Line(s) 558 (Use of self.fail())
tests/test_client: Line(s) 589 (Use of self.fail())
tests/test_client: Line(s) 609 (Use of self.fail())
tests/test_client: Line(s) 735 (Use of self.fail())
tests/test_client: Line(s) 842 (Use of self.fail())
tests/test_client: Line(s) 941 (Use of self.fail())
tests/test_client: Line(s) 944-945 (Use of self.fail() and an Exception that never occurs)
tests/test_client: Line(s) 1104 (Use of self.fail())
tests/test_client: Line(s) 1116 (Use of self.fail())
tests/test_client: Line(s) 1136 (Use of self.fail())
tests/test_client: Line(s) 1256 (Use of self.fail())
tests/test_client: Line(s) 1333 (Use of self.fail())
tests/test_client: Line(s) 1372 (Use of self.fail())
tests/test_client: Line(s) 1420 (unittest.main() at end of file)
tests/test_clientsecrets: Line(s) 212-213 (catch AttributeError that doesn't occur)
tests/test_clientsecrets: Line(s) 217 (Use of self.fail())
tests/test_clientsecrets: Line(s) 225 (Use of self.fail())
tests/test_clientsecrets: Line(s) 232 (Use of self.fail())
tests/test_clientsecrets: Line(s) 286 (Use of self.fail())
tests/test_clientsecrets: Line(s) 299 (unittest.main() at end of file)
tests/test_devshell: Line(s) 143 (YAGNI exception in _AuthReferenceServer.run)
tests/test_devshell: Line(s) 149 (YAGNI branch for longer message in _AuthReferenceServer.run)
tests/test_devshell: Line(s) 151 (Exception that never occurs)
tests/test_django_orm: Line(s) 32 (Try to import AppEngine libs)
tests/test_django_orm: Line(s) 92 (unittest.main() at end of file)
tests/test_file: Line(s) 44-45 (catch ImportError in Py3)
tests/test_file: Line(s) 93 (Use of self.fail())
tests/test_file: Line(s) 278 (Use of self.fail())
tests/test_file: Line(s) 419 (unittest.main() at end of file)
tests/test_flask_util: Line(s) 381 (Return statement that never executes)
tests/test_flask_util: Line(s) 430 (unittest.main() at end of file)
tests/test_jwt: Line(s) 91 (Use of self.fail())
tests/test_jwt: Line(s) 328 (Use of self.fail())
tests/test_jwt: Line(s) 341 (unittest.main() at end of file)
tests/test_service_account: Line(s) 66 (Use of self.fail())
tests/test_service_account: Line(s) 72 (Use of self.fail())
tests/test_tools: Line(s) 31 (unittest.main() at end of file)
tests/test_xsrfutil: Line(s) 296 (unittest.main() at end of file)


I have gone through by hand and classified them.

@dhermes dhermes force-pushed the adding-tests-to-coverage branch from 44e96b6 to 84a5d09 Compare September 3, 2015 17:50
@nathanielmanistaatgoogle
Copy link
Contributor

The two examples of self.fail you linked would be best addressed by changing their sites into with self.assertRaises blocks, but that would break 2.6 compatibility. Where are we using self.fail outside of an expecting-an-exception context?

@dhermes
Copy link
Contributor Author

dhermes commented Sep 3, 2015

There are 29 uses of self.fail() and all of them except for test_client.py#L944-L945 occur right before an except statement.

As for Py2.6 support, AFAIK unittest2 is what most people use.

@nathanielmanistaatgoogle
Copy link
Contributor

That odd one out that you found looks superfluous and like it could be eliminated today without affecting the correctness or quality of the test.

As for the others if I wanted to make a case for addressing them I'm not sure whether I'd favor making use of unittest2 or dropping support for 2.6. :-) 2.6 is really, really old...

nathanielmanistaatgoogle added a commit that referenced this pull request Sep 3, 2015
@nathanielmanistaatgoogle nathanielmanistaatgoogle merged commit df5bacc into googleapis:master Sep 3, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Sep 3, 2015

I agree it is superfluous. The majority of these uncovered lines are superfluous / YAGNI.


See googleapis/google-cloud-python#995 for my thoughts on supporting Py2.6.

In particular, Oct. 29 2013 "retired" Py2.6 (retired by core-dev) and Nick Coghlan (a core dev) made a compelling argument for dropping Py2.6 support:
http://www.curiousefficiency.org/posts/2015/04/stop-supporting-python26.html

He actually said since Google pays people to support these libraries, his advice doesn't fully translate over.

@dhermes dhermes deleted the adding-tests-to-coverage branch September 3, 2015 19:14
@dhermes
Copy link
Contributor Author

dhermes commented Sep 3, 2015

@nathanielmanistaatgoogle Should I file a "Drop support for Python 2.6" issue for oauth2client?

@nathanielmanistaatgoogle
Copy link
Contributor

Sure. I don't know what the answer will be but it's worth at least asking the question.

This was referenced Sep 3, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Sep 22, 2015

Updated summary of test lines missing coverage as of 3b6b4be:

tests/test_appengine: Branch miss
tests/test_client: Line(s) 305 (Use of self.fail())
tests/test_client: Line(s) 410 (Use of self.fail())
tests/test_client: Line(s) 425 (Use of self.fail())
tests/test_client: Line(s) 445 (Use of self.fail())
tests/test_client: Line(s) 460 (Use of self.fail())
tests/test_client: Line(s) 500 (Use of self.fail())
tests/test_client: Line(s) 522 (Use of self.fail())
tests/test_client: Line(s) 553 (Use of self.fail())
tests/test_client: Line(s) 573 (Use of self.fail())
tests/test_client: Line(s) 700 (Use of self.fail())
tests/test_client: Line(s) 807 (Use of self.fail())
tests/test_client: Line(s) 906 (Use of self.fail())
tests/test_client: Line(s) 1067 (Use of self.fail())
tests/test_client: Line(s) 1079 (Use of self.fail())
tests/test_client: Line(s) 1099 (Use of self.fail())
tests/test_client: Line(s) 1219 (Use of self.fail())
tests/test_client: Line(s) 1296 (Use of self.fail())
tests/test_client: Line(s) 1335 (Use of self.fail())
tests/test_clientsecrets: Line(s) 215 (Use of self.fail())
tests/test_clientsecrets: Line(s) 223 (Use of self.fail())
tests/test_clientsecrets: Line(s) 230 (Use of self.fail())
tests/test_clientsecrets: Line(s) 284 (Use of self.fail())
tests/test_django_orm: Line(s) 32 (Try to import AppEngine libs)
tests/test_file: Line(s) 44-45 (catch ImportError in Py3)
tests/test_file: Line(s) 93 (Use of self.fail())
tests/test_file: Line(s) 278 (Use of self.fail())
tests/test_flask_util: Line(s) 381 (Return statement that never executes)
tests/test_jwt: Line(s) 91 (Use of self.fail())
tests/test_jwt: Line(s) 328 (Use of self.fail())
tests/test_service_account: Line(s) 66 (Use of self.fail())
tests/test_service_account: Line(s) 72 (Use of self.fail())

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

Successfully merging this pull request may close these issues.

3 participants