Fixes #297 New unit tests based on responses

cash commented

2014-01-11 17:04:20 -08:00

(Migrated from github.com)

This is ready for review. I'm going to stop work on it until I have feedback.

Added a set of unit tests for creating requests using dropbox/responses so no external dependencies
Moved the endpoint integration tests to test_endpoints.py
Skipped the tests that have been failing. Maybe the OAuth token is not good anymore (a good reason to update them to use a stub or Betamax) Link to an example of the tests failing (82 of them!): https://travis-ci.org/ryanmcgrath/twython/builds/16699968
Made updates to the new unit tests to get them to run on 2.6, 2.7, and 3.3
Removed the unit test for cursor - it needs to be rewritten. I'll work on that next.

The test coverage dropped to 55%, but most of the tests that I skipped were sanity tests - they did not test anything specific, but made sure no exceptions were thrown.

This is ready for review. I'm going to stop work on it until I have feedback. 1. Added a set of unit tests for creating requests using dropbox/responses so no external dependencies 2. Moved the endpoint integration tests to test_endpoints.py 3. Skipped the tests that have been failing. Maybe the OAuth token is not good anymore (a good reason to update them to use a stub or Betamax) Link to an example of the tests failing (82 of them!): https://travis-ci.org/ryanmcgrath/twython/builds/16699968 4. Made updates to the new unit tests to get them to run on 2.6, 2.7, and 3.3 5. Removed the unit test for cursor - it needs to be rewritten. I'll work on that next. The test coverage dropped to 55%, but most of the tests that I skipped were sanity tests - they did not test anything specific, but made sure no exceptions were thrown.

coveralls commented

2014-01-11 17:05:23 -08:00

(Migrated from github.com)

Changes Unknown when pulling 9f7d38181e on cash:unit_tests into * on ryanmcgrath:master*.

[![Coverage Status](https://coveralls.io/builds/434105/badge)](https://coveralls.io/builds/434105) Changes Unknown when pulling **9f7d38181ed65b0bba05d205689877cd9d70f4d9 on cash:unit_tests** into *\* on ryanmcgrath:master**.

michaelhelmick commented

2014-01-12 08:44:55 -08:00

(Migrated from github.com)

I'll look over these pull requests either later today or tomorrow! Thanks for your work! :D—
Sent from Mailbox for iPhone

On Sat, Jan 11, 2014 at 8:04 PM, Cash Costello notifications@github.com
wrote:

This is ready for review. I'm going to stop work on it until I have feedback.

Added a set of unit tests for creating requests using dropbox/responses so no external dependencies

Moved the endpoint integration tests to test_endpoints.py

Skipped the tests that have been failing. Maybe the OAuth token is not good anymore (a good reason to update them to use a stub or Betamax) Link to an example of the tests failing (82 of them!): https://travis-ci.org/ryanmcgrath/twython/builds/16699968

Made updates to the new unit tests to get them to run on 2.6, 2.7, and 3.3

Removed the unit test for cursor - it needs to be rewritten. I'll work on that next.
The test coverage dropped to 55%, but most of the tests that I skipped were sanity tests - they did not test anything specific, but made sure no exceptions were thrown.
You can merge this Pull Request by running:
git pull https://github.com/cash/twython unit_tests
Or you can view, comment on it, or merge it online at:
https://github.com/ryanmcgrath/twython/pull/303
-- Commit Summary --

added some basic tests of the core request() method using dropbox/responses

moved integration tests to new file

added docstrings for new tests and finished testing input arguments of request()

updated remaining tests in test_core.py and removed cursor test (which will be replaced)

skipping tests that were failing because of external dependency on Twitter

updated unit tests and travis config for python 2.6 and 3

python 3.3 workaround for dropbox/responses issue
-- File Changes --
M .travis.yml (4)
M requirements.txt (1)
M tests/config.py (6)
M tests/test_auth.py (13)
M tests/test_core.py (669)
A tests/test_endpoints.py (503)
M tests/test_streaming.py (9)
-- Patch Links --
https://github.com/ryanmcgrath/twython/pull/303.patch
https://github.com/ryanmcgrath/twython/pull/303.diff
Reply to this email directly or view it on GitHub:
https://github.com/ryanmcgrath/twython/pull/303

I'll look over these pull requests either later today or tomorrow! Thanks for your work! :D— Sent from Mailbox for iPhone On Sat, Jan 11, 2014 at 8:04 PM, Cash Costello notifications@github.com wrote: > This is ready for review. I'm going to stop work on it until I have feedback. > 1. Added a set of unit tests for creating requests using dropbox/responses so no external dependencies > 2. Moved the endpoint integration tests to test_endpoints.py > 3. Skipped the tests that have been failing. Maybe the OAuth token is not good anymore (a good reason to update them to use a stub or Betamax) Link to an example of the tests failing (82 of them!): https://travis-ci.org/ryanmcgrath/twython/builds/16699968 > 4. Made updates to the new unit tests to get them to run on 2.6, 2.7, and 3.3 > 5. Removed the unit test for cursor - it needs to be rewritten. I'll work on that next. > The test coverage dropped to 55%, but most of the tests that I skipped were sanity tests - they did not test anything specific, but made sure no exceptions were thrown. > You can merge this Pull Request by running: > git pull https://github.com/cash/twython unit_tests > Or you can view, comment on it, or merge it online at: > https://github.com/ryanmcgrath/twython/pull/303 > -- Commit Summary -- > - added some basic tests of the core request() method using dropbox/responses > - moved integration tests to new file > - added docstrings for new tests and finished testing input arguments of request() > - updated remaining tests in test_core.py and removed cursor test (which will be replaced) > - skipping tests that were failing because of external dependency on Twitter > - updated unit tests and travis config for python 2.6 and 3 > - python 3.3 workaround for dropbox/responses issue > -- File Changes -- > M .travis.yml (4) > M requirements.txt (1) > M tests/config.py (6) > M tests/test_auth.py (13) > M tests/test_core.py (669) > A tests/test_endpoints.py (503) > M tests/test_streaming.py (9) > -- Patch Links -- > https://github.com/ryanmcgrath/twython/pull/303.patch > https://github.com/ryanmcgrath/twython/pull/303.diff > --- > Reply to this email directly or view it on GitHub: > https://github.com/ryanmcgrath/twython/pull/303

michaelhelmick commented

2014-01-13 11:02:39 -08:00

(Migrated from github.com)

Those tests will fail because Travis can't decrypt encrypted secret environment variables of branches from another origin. (i.e. if I create a branch ryanmcgrath/twython/patch-1 tests will be fine, but your branch cash/twython/unit_tests won't be because it's of another origin) Either responses or BetaMax would work to help mock the endpoint tests. The tokens are encrypted currently so that nobody (besides who encrypted them) actually knows the tokens, wouldn't want spammers spamming up the @__twython__ Twitter account haha

https://github.com/ryanmcgrath/twython/pull/303/files#diff-cc4345db19ff44863e9122c74e9f383fR55 Seems a little off, that URL doesn't exist (they haven't released a 2.0 API)

But overall, good work. Thanks!

1. Those tests will fail because Travis can't decrypt encrypted secret environment variables of branches from another origin. (i.e. if I create a branch `ryanmcgrath/twython/patch-1` tests will be fine, but your branch `cash/twython/unit_tests` won't be because it's of another origin) Either `responses` or `BetaMax` would work to help mock the `endpoint` tests. The tokens are encrypted currently so that nobody (besides who encrypted them) actually knows the tokens, wouldn't want spammers spamming up the `@__twython__` Twitter account haha https://github.com/ryanmcgrath/twython/pull/303/files#diff-cc4345db19ff44863e9122c74e9f383fR55 Seems a little off, that URL doesn't exist (they haven't released a 2.0 API) But overall, good work. Thanks!

cash commented

2014-01-13 11:52:04 -08:00

(Migrated from github.com)

Here is the test of interest:

    @responses.activate
    def test_request_should_handle_relative_endpoint(self):
        """Test that request() accepts a twitter endpoint name for the endpoint argument"""
        url = 'https://api.twitter.com/2.0/search/tweets.json'
        self.register_response(responses.GET, url)

        self.api.request('search/tweets', version='2.0')

        self.assertEqual(1, len(responses.calls))
        self.assertEqual(url, responses.calls[0].request.url)

This tests whether an endpoint is mapped to a correct full URL. The API version is one of the parameters and I wanted to exercise a non-default parameter (1.1 is default). I could change it to 1.0 if you prefer. Either way, it is testing the proper creation of the URL rather than testing Twitter's endpoints. Is this the only thing holding up acceptance?

Here is the test of interest: ``` @responses.activate def test_request_should_handle_relative_endpoint(self): """Test that request() accepts a twitter endpoint name for the endpoint argument""" url = 'https://api.twitter.com/2.0/search/tweets.json' self.register_response(responses.GET, url) self.api.request('search/tweets', version='2.0') self.assertEqual(1, len(responses.calls)) self.assertEqual(url, responses.calls[0].request.url) ``` This tests whether an endpoint is mapped to a correct full URL. The API version is one of the parameters and I wanted to exercise a non-default parameter (1.1 is default). I could change it to 1.0 if you prefer. Either way, it is testing the proper creation of the URL rather than testing Twitter's endpoints. Is this the only thing holding up acceptance?

coveralls commented

2014-01-14 08:34:31 -08:00

(Migrated from github.com)

Changes Unknown when pulling c83304edf2 on cash:unit_tests into * on ryanmcgrath:master*.

[![Coverage Status](https://coveralls.io/builds/439672/badge)](https://coveralls.io/builds/439672) Changes Unknown when pulling **c83304edf27514994bb66cb2d1b2056d9d1893e5 on cash:unit_tests** into *\* on ryanmcgrath:master**.

cash commented

2014-01-14 08:44:12 -08:00

(Migrated from github.com)

@michaelhelmick ready to go

michaelhelmick commented

2014-01-14 08:45:47 -08:00

(Migrated from github.com)

Yeah, other than that, it all looks good. And before we make a release, I'd like to get coverage above 80%. Again, thanks for this and I will merge this sometime later today or tomorrow! I have a busy schedule today 🍰

Yeah, other than that, it all looks good. And before we make a release, I'd like to get coverage above 80%. Again, thanks for this and I will merge this sometime later today or tomorrow! I have a busy schedule today :cake:

michaelhelmick commented

2014-01-17 18:23:47 -08:00

(Migrated from github.com)

I'll get around tomorrow to merging this, just have had a busy week! :D

cash commented

2014-01-20 10:39:02 -08:00

(Migrated from github.com)

What do you want to do before you merge this? I don't see anything else to do given the Travis CI results.

michaelhelmick commented

2014-01-20 10:44:27 -08:00

(Migrated from github.com)

Nothing, I’ve just been real busy and I wanted to look over it one more time before I merge it in. Sorry for the wait :(

--
Mike Helmick
Software Engineer
P: (330) 507-9098
T: @mikehelmick
W: https://github.com/michaelhelmick

On January 20, 2014 at 1:39:38 PM, Cash Costello (notifications@github.com) wrote:

What do you want to do before you merge this? I don't see anything else to do given the Travis CI results.

—
Reply to this email directly or view it on GitHub.

Nothing, I’ve just been real busy and I wanted to look over it one more time before I merge it in. Sorry for the wait :( -- Mike Helmick Software Engineer P: (330) 507-9098 T: @mikehelmick W: https://github.com/michaelhelmick On January 20, 2014 at 1:39:38 PM, Cash Costello (notifications@github.com) wrote: What do you want to do before you merge this? I don't see anything else to do given the Travis CI results. — Reply to this email directly or view it on GitHub.

michaelhelmick commented

2014-01-25 08:03:18 -08:00

(Migrated from github.com)

Finally had time to look over this a final time before merge, all seemed good to go! Thank you for your work @cash -- hopefully we'll see more from you! 👍

Finally had time to look over this a final time before merge, all seemed good to go! Thank you for your work @cash -- hopefully we'll see more from you! :+1:

Rows
Columns

Fixes #297 New unit tests based on responses #303