Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

configurable url, use bypass for api-tests #38

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Conversation

ReneKl
Copy link

@ReneKl ReneKl commented May 16, 2019

make url configurable for api-tests with bypass lib,
timezone tests could not be fixed (also in doctest)

@sntran
Copy link
Owner

sntran commented May 16, 2019

Hi there,

First, thank you for contributing! I really appreciate your efforts into it.

However, I do notice you have been experiencing issues getting the tests to work implementing this PR. Do you think it would be better to raise an issue with what you are encountering, and we can work out the details. Or would you like me to review this PR now?

@nbw
Copy link

nbw commented Dec 6, 2019

@sntran Seems like the tests for this one are now passing. Any chance we could get this one merged?

@sntran
Copy link
Owner

sntran commented Dec 6, 2019

Hi @nbw .

With the lack of communication from @ReneKl , it's hard for me to figure out what this PR does. I have spent some time looking through the changes, and here are a few concerns I have:

  • The PR is based on the dev branch, which switched to mint from httpoison. The PR then changed from mint back to httpoision. This is hard for me to merge.
  • Pardon my ignorance, but I don't fully understand the purpose of bypass. If you are to mock API response to your desired, then how the tests would correctly corresponding to actual API response? Yes, the tests will pass, but the library will fail if Google's response is difference.
  • The PR also commented out some tests that do not pass for itself. If the purpose of the PR is to provide more test cases, removing other test cases are not ideal.

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

Successfully merging this pull request may close these issues.

3 participants