Ensure query string is included when building _api URL. Fixes #672 #673
Conversation
| @@ -52,7 +52,7 @@ class TestGitHubCore(helper.UnitHelper): | |||
| last_modified = datetime.now().strftime( | |||
| '%a, %d %b %Y %H:%M:%S GMT' | |||
| ) | |||
| url = 'https://api.github.com/foo' | |||
| url = 'https://api.github.com/foo?bar=1' | |||
There was a problem hiding this comment.
Please create a separate test class for this test and revert this change.
|
Also looks like this will break some of our recorded cassettes. Once you've addressed my review feedback above, I'll push new cassettes to your branch. |
|
OK, I assume that's what the build failures are regarding? |
|
Yep. That's why those are failing. Thanks for updating. I'll push a new commit with updated cassettes here in a few. |
Conflicts:
AUTHORS.rst
|
Ha, a bunch of my automation has broken as well. Would it be better if we didn't add the query if it was just "ref=master"? |
|
I'd rather not special case this, personally. How did your automation break? |
|
I fake the GitHub API to make a bunch of our tests run faster. So, for example, if it was expecting a GET to Not a problem to fix the tests, just thought I'd check. Agreed it's not pretty to add that special case. |
Just also not practical. Once these tests pass, we should be good to merge this. Thanks for finding, reporting, and fixing this @dprothero ! |
Now that we include the query string in the URL, we need to update our cassettes and some of our other unit tests. This also fixes our tox.ini to work with newer versions of tox (i.e., passenv)
Fixes Issue #672