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

More informative error when failing to open secrets file. #350

Merged
merged 1 commit into from
Dec 4, 2015

Conversation

waprin
Copy link
Contributor

@waprin waprin commented Dec 3, 2015

Django 1.9 should work so py27 should work at least.

Fixes #348.

@googlebot
Copy link
Collaborator

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.


SECRET_KEY = 'this string is not a real django secret key'
DATABASES = {
'default': {

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Dec 3, 2015

Thanks @waprin. I didn't realize that support for Python 2.6 was dropped in django==1.7 along with the fact that support for Python 3.3 was dropped in django==1.9.

Maybe we should just turn off the Django tests for py26 and py33?

@nathanielmanistaatgoogle WDYT?

@dhermes
Copy link
Contributor

dhermes commented Dec 3, 2015

The easiest way to fix the absence of the self.assertRaises context manager in unittest would be to upgrade to unittest2, but I just manually catch and save the exception. (Is a little funky since in Py3k caught exceptions don't stay in scope.)

Other than that I just turned off django in Python2.6,3.3 and made a note about it in the CONTRIBUTING.md doc (so that people know why the tests are off).

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 499baba..85599e2 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -125,6 +125,10 @@ Running Tests
     least version 2.6 of `pypy` installed. See the [docs][13] for
     more information.

+-   **Note** that `django` related tests are turned off for Python 2.6
+    and 3.3. This is because `django` dropped support for
+    [2.6 in `django==1.7`][14] and for [3.3 in `django==1.9`][15].
+
 Running System Tests
 --------------------

@@ -196,3 +200,5 @@ we'll be able to accept your pull requests.
 [11]: #include-tests
 [12]: #make-the-pull-request
 [13]: http://oauth2client.readthedocs.org/en/latest/#using-pypy
+[14]: https://docs.djangoproject.com/en/1.7/faq/install/#what-python-version-can-i-use-with-django
+[15]: https://docs.djangoproject.com/en/1.9/faq/install/#what-python-version-can-i-use-with-django
diff --git a/tests/test_clientsecrets.py b/tests/test_clientsecrets.py
index 2beb3bb..7803fd7 100644
--- a/tests/test_clientsecrets.py
+++ b/tests/test_clientsecrets.py
@@ -225,17 +225,17 @@ class OAuth2CredentialsTests(unittest.TestCase):
                 self.assertTrue(str(e).startswith(match))

     def test_load_by_filename_missing_file(self):
-        exc_manager = None
-        with self.assertRaises(
-                clientsecrets.InvalidClientSecretsError) as exc_manager:
+        exc_caught = None
+        try:
             clientsecrets._loadfile(NONEXISTENT_FILE)
-
-        exc = exc_manager.exception
-        self.assertEqual(len(exc.args), 4)
-        self.assertEqual(exc.args[0], 'Error opening file')
-        self.assertEqual(exc.args[1], NONEXISTENT_FILE)
-        self.assertEqual(exc.args[2], 'No such file or directory')
-        self.assertEqual(exc.args[3], errno.ENOENT)
+        except clientsecrets.InvalidClientSecretsError as exc:
+            exc_caught = exc
+
+        expected_args = ('Error opening file',
+                         NONEXISTENT_FILE,
+                         'No such file or directory',
+                         errno.ENOENT)
+        self.assertEqual(exc_caught.args, expected_args)


 class CachedClientsecretsTests(unittest.TestCase):
diff --git a/tox.ini b/tox.ini
index 8993654..11eafa7 100644
--- a/tox.ini
+++ b/tox.ini
@@ -42,6 +42,28 @@ deps = {[testenv]deps}
     coverage
     nosegae

+[testenv:py26]
+basepython =
+    python2.6
+commands =
+    nosetests \
+      --ignore-files=test_appengine\.py \
+      --ignore-files=test_django_orm\.py \
+      --ignore-files=test_django_settings\.py \
+      {posargs}
+deps = {[testenv]basedeps}
+
+[testenv:py33]
+basepython =
+    python3.3
+commands =
+    nosetests \
+      --ignore-files=test_appengine\.py \
+      --ignore-files=test_django_orm\.py \
+      --ignore-files=test_django_settings\.py \
+      {posargs}
+deps = {[testenv]basedeps}
+
 [testenv:cover]
 basepython = {[coverbase]basepython}
 commands =
@@ -72,11 +94,6 @@ deps =
     webapp2
 commands = {toxinidir}/scripts/build-docs

-[testenv:py26]
-basepython = python2.6
-deps = {[testenv]basedeps}
-       django>=1.5,<1.6
-
 [testenv:gae]
 basepython = python2.7
 deps = {[testenv]basedeps}

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@waprin
Copy link
Contributor Author

waprin commented Dec 3, 2015

Applied your diff, originally thought you were going to cherry-pick my commit into yours. Squashed it (or keep as separate commit? Django stuff is kind of unrelated). Think Travis will pass now but let's see. I definitely think disabling Django tests for 2.6/3.3 is the right thing to do.

'ENGINE': 'django.db.backends.sqlite3',
'NAME': os.path.join('.', 'db.sqlite3'),
}
}

This comment was marked as spam.

@dhermes dhermes removed the cla: no label Dec 3, 2015
@dhermes
Copy link
Contributor

dhermes commented Dec 3, 2015

Thanks @waprin. No worries on a cherry pick or H/T, you did more thinking here than I did.

LGTM to merge

I won't to hold off until @nathanielmanistaatgoogle chimes on on turning off the django tests for Py2.6 and Py3.3.

@nathanielmanistaatgoogle
Copy link
Contributor

@dhermes: I think it adds fuel to the fire of #298. I am completely okay with turning off Django tests for versions of Python unsupported by Django.



class DjangoOrmTestApp(AppConfig):
""" App Config for Django Helper"""

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Dec 3, 2015

I'm all for adding fuel to that fire!

@waprin Ping me once the space is fixed / squashed into the original commit and the tests pass, and I'll be happy to merge.

@@ -32,7 +33,8 @@
DATA_DIR = os.path.join(os.path.dirname(__file__), 'data')
VALID_FILE = os.path.join(DATA_DIR, 'client_secrets.json')
INVALID_FILE = os.path.join(DATA_DIR, 'unfilled_client_secrets.json')
NONEXISTENT_FILE = os.path.join(__file__, '..', 'afilethatisntthere.json')
NONEXISTENT_FILE = os.path.join(os.path.dirname(__file__),

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
Copy link
Contributor

dhermes commented Dec 3, 2015

FYI @waprin I added the "Fixes" to the description so that GitHub can automatically close the bug that was filed.

@@ -222,12 +224,18 @@ def test_validate_error(self):
except clientsecrets.InvalidClientSecretsError as e:
self.assertTrue(str(e).startswith(match))

def test_load_by_filename(self):
def test_load_by_filename_missing_file(self):
exc_caught = None

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Fixes googleapis#348.

Fix Django ORM Tests for Django 1.9.

Disables Django tests for 2.6 and 3.3 due to lack of Django support.
@dhermes
Copy link
Contributor

dhermes commented Dec 3, 2015

Ping me when
https://travis-ci.org/google/oauth2client/builds/94749565
passes and I'll be happy to merge for you.

@nathanielmanistaatgoogle
Copy link
Contributor

There's currently a bright red "cla: no" label on this pull request; that absolutely must be addressed and ameliorated before this goes in. It's probably something minor like one of you authored or committed with an email address that has not signed the CLA.

Change content looks good to me.

@dhermes
Copy link
Contributor

dhermes commented Dec 4, 2015

The issue is that @waprin push a commit with my email address on it:

https://api.github.com/repos/google/oauth2client/pulls/350/commits

      "author": {
        "name": "Danny Hermes",
        "email": "[email protected]",
        "date": "2015-12-02T04:42:52Z"
      },
      "committer": {
        "name": "Bill Prin",
        "email": "[email protected]",
        "date": "2015-12-03T22:14:37Z"
      },

The Google license checker is too dumb for that situation. Fails every time.

@nathanielmanistaatgoogle
Copy link
Contributor

I've manually verified CLA signage for both of you. Merging.

nathanielmanistaatgoogle added a commit that referenced this pull request Dec 4, 2015
More informative error when failing to open secrets file.
@nathanielmanistaatgoogle nathanielmanistaatgoogle merged commit b2a4055 into googleapis:master Dec 4, 2015
dhermes added a commit to dhermes/oauth2client that referenced this pull request Dec 5, 2015
In particular

- Upgraded some unit test modules to `unittest2`.
- Got to 100% line coverage in `test_jwt`, `test_clientsecrets`,
  and `test_service_account` by using `self.assertRaises`
  instead of `self.fail()`
- In some places used `self.assertRaises` as a context manager so
  the thrown exception can be inspected / compared against. This
  is done instead of manually catching the exception and
  holding on to it (`unittest2` backports this feature from
  `unittest` that was not available in Python 2.6)
- Added license to `test_multistore_file`
- Removed legacy `python2.4` hashbang from `test_jwt` and
  `test_service_account`
- Added `unittest2` as a test dependency in `tox.ini`
- Updated `test_clientsecrets.test_load_by_filename_missing_file`
  to use the `self.assertRaises` context manager (this was
  a problem in googleapis#349 and googleapis#350)
- Updated `test_jwt` module docstring to reflect actual purpose
dhermes added a commit to dhermes/oauth2client that referenced this pull request Dec 14, 2015
In particular

- Upgraded some unit test modules to `unittest2`.
- Got to 100% line coverage in `test_jwt`, `test_clientsecrets`,
  and `test_service_account` by using `self.assertRaises`
  instead of `self.fail()`
- In some places used `self.assertRaises` as a context manager so
  the thrown exception can be inspected / compared against. This
  is done instead of manually catching the exception and
  holding on to it (`unittest2` backports this feature from
  `unittest` that was not available in Python 2.6)
- Added license to `test_multistore_file`
- Removed legacy `python2.4` hashbang from `test_jwt` and
  `test_service_account`
- Added `unittest2` as a test dependency in `tox.ini`
- Updated `test_clientsecrets.test_load_by_filename_missing_file`
  to use the `self.assertRaises` context manager (this was
  a problem in googleapis#349 and googleapis#350)
- Updated `test_jwt` module docstring to reflect actual purpose
dhermes added a commit to dhermes/oauth2client that referenced this pull request Dec 14, 2015
In particular

- Upgraded some unit test modules to `unittest2`.
- Got to 100% line coverage in `test_jwt`, `test_clientsecrets`,
  and `test_service_account` by using `self.assertRaises`
  instead of `self.fail()`
- In some places used `self.assertRaises` as a context manager so
  the thrown exception can be inspected / compared against. This
  is done instead of manually catching the exception and
  holding on to it (`unittest2` backports this feature from
  `unittest` that was not available in Python 2.6)
- Added license to `test_multistore_file`
- Removed legacy `python2.4` hashbang from `test_jwt` and
  `test_service_account`
- Added `unittest2` as a test dependency in `tox.ini`
- Updated `test_clientsecrets.test_load_by_filename_missing_file`
  to use the `self.assertRaises` context manager (this was
  a problem in googleapis#349 and googleapis#350)
- Updated `test_jwt` module docstring to reflect actual purpose
@dhermes dhermes mentioned this pull request Jan 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants