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

Fix request timeout #188

Merged
merged 2 commits into from
Aug 26, 2019
Merged

Conversation

makoto-matsumoto
Copy link
Contributor

Changes

#170 changed the initialization of the Auth0::Exception class, but Auth0::RequestTimeout was not changed.

References

Testing

  • This change adds unit test coverage
  • This change adds integration test coverage
  • This change has been tested on the latest version of Ruby

Checklist

@makoto-matsumoto makoto-matsumoto requested a review from a team August 20, 2019 01:17
@joshcanhelp
Copy link
Contributor

@makoto-matsumoto - good catch, thank you for this. Is this currently broken when a RequestTimeout is raised or is this just a nice-to-have including the message there? Asking because I'm not sure if this warrants a patch release.

@makoto-matsumoto
Copy link
Contributor Author

@joshcanhelp
This is currently broken.

lib/auth0/mixins/httpproxy.rb

      rescue RestClient::Exception => e
        case e
        when RestClient::RequestTimeout
          raise Auth0::RequestTimeout
        else

spec/lib/auth0/mixins/httpproxy_spec.rb

  1) Auth0::Mixins::HTTPProxy.get should raise Auth0::RequestTimeout on send http get method
        to path defined through HTTP when RestClient::RequestTimeout received
     Failure/Error: expect { @instance.send(http_method, '/test') }.to raise_error(Auth0::RequestTimeout)

       expected Auth0::RequestTimeout, got #<ArgumentError: wrong number of arguments (given 0, expected 1..2)> with backtrace:
         # ./lib/auth0/exception.rb:7:in `initialize'
         # ./lib/auth0/mixins/httpproxy.rb:71:in `exception'
         # ./lib/auth0/mixins/httpproxy.rb:71:in `raise'
         # ./lib/auth0/mixins/httpproxy.rb:71:in `rescue in call'
         # ./lib/auth0/mixins/httpproxy.rb:60:in `call'
         # ./lib/auth0/mixins/httpproxy.rb:20:in `block (2 levels) in <module:HTTPProxy>'
         # ./spec/lib/auth0/mixins/httpproxy_spec.rb:77:in `block (5 levels) in <top (required)>'
         # ./spec/lib/auth0/mixins/httpproxy_spec.rb:77:in `block (4 levels) in <top (required)>'
     # ./spec/lib/auth0/mixins/httpproxy_spec.rb:77:in `block (4 levels) in <top (required)>'

Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@joshcanhelp joshcanhelp merged commit 279093e into auth0:master Aug 26, 2019
@zinosama
Copy link

Hi @joshcanhelp could you please do a patch release for this? It's currently broken and has caused some confusion for my team. Thanks a lot!

@joshcanhelp
Copy link
Contributor

@zinosama - Apologies for the delay here! I'll cut a release in the next day or so. Thank you for the reminder 👍

@joshcanhelp joshcanhelp added this to the v4.9.0 milestone Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants