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

Removing test_import.py. #288

Merged

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Aug 25, 2015

It's unclear what the purpose ever was.

It is no longer needed. Original intent is now covered by test_tools.


@nathanielmanistaatgoogle I came across this while working on #212 over lunch. I also realized that we don't have a perfect mapping of modules to tests:

tests/test_appengine.py       --> oauth2client/appengine.py
                              --> oauth2client/client.py
tests/test_clientsecrets.py   --> oauth2client/clientsecrets.py
tests/test_crypt.py           --> oauth2client/crypt.py
tests/test_devshell.py        --> oauth2client/devshell.py
tests/test_django_orm.py      --> oauth2client/django_orm.py
tests/test_file.py            --> oauth2client/file.py
tests/test_flask_util.py      --> oauth2client/flask_util.py
tests/test_gce.py             --> oauth2client/gce.py
tests/test__helpers.py        --> oauth2client/_helpers.py
                              --> oauth2client/__init__.py
tests/test_import.py          -->
tests/test_jwt.py             -->
tests/test_keyring_storage.py --> oauth2client/keyring_storage.py
                              --> oauth2client/locked_file.py
                              --> oauth2client/multistore_file.py
tests/test_oauth2client.py    -->
                              --> oauth2client/old_run.py
                              --> oauth2client/_openssl_crypt.py
tests/test__pycrypto_crypt.py --> oauth2client/_pycrypto_crypt.py
tests/test_service_account.py --> oauth2client/service_account.py
tests/test_tools.py           --> oauth2client/tools.py
tests/test_util.py            --> oauth2client/util.py
tests/test_xsrfutil.py        --> oauth2client/xsrfutil.py

Shall I file an issue?

@nathanielmanistaatgoogle
Copy link
Contributor

I think it's perfectly clear what test_import's purpose ever was from its file history - did you dig into it in the course of preparing this change?

Now I believe that test_import's function is redundant with that of test_tools and that test_import can be removed, but please update your commit message to reflect that? Or disagree, if you disagree, of course.

As for filing an issue for a one-to-one mapping between source modules and test modules: I think I'd be tempted to dispute the premise of such a plan. I think I'm fine to live in a world in which tests map one-to-one with behaviors under test and code maps one-to-one with... something else that isn't necessarily behaviors afforded by the code. Is there any reason I shouldn't want to live in that world?

@dhermes
Copy link
Contributor Author

dhermes commented Aug 26, 2015

  • I did not dig into history
  • I am happy to change the commit and PR descriptions
  • I don't know if the 1-1 mapping is crucial, but it seems this library has just done it in a haphazard way. (For example test_oauth2client.py is so named because oauth2client used to be part of google-api-python-client.)

It is no longer needed. Original intent is now covered by
test_tools.
@dhermes
Copy link
Contributor Author

dhermes commented Aug 26, 2015

@nathanielmanistaatgoogle I updated the commit and PR descriptions. PTAL

LMK what you think about the mapping. I think at the very least we should

  • Rename test_oauth2client.py -> test_client.py
  • 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 (you may disagree with me on this)

nathanielmanistaatgoogle added a commit that referenced this pull request Aug 26, 2015
@nathanielmanistaatgoogle nathanielmanistaatgoogle merged commit 14a1229 into googleapis:master Aug 26, 2015
@dhermes dhermes deleted the remove-unused-test branch August 26, 2015 13:48
@nathanielmanistaatgoogle
Copy link
Contributor

I think one-test-module-to-one-source-module is fine, I think many-test-modules-to-one-source-module is fine, and I think one-test-module-to-many-source-modules is almost certainly not fine. I suspect you probably feel similarly? All three items in your list look like favorably judicious actions.

@dhermes
Copy link
Contributor Author

dhermes commented Aug 26, 2015

OK cool. The first is trivial and the other two are less so (require more than 20 seconds to make happen). I'll just mention them in #212 and hope I don't forget 😀

This was referenced Aug 26, 2015
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