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

Spec/Implementation for "open/read timeout" needs to be improved #17

Open
walro opened this issue Dec 3, 2020 · 1 comment
Open

Spec/Implementation for "open/read timeout" needs to be improved #17

walro opened this issue Dec 3, 2020 · 1 comment
Labels
enhancement New feature or request

Comments

@walro
Copy link
Contributor

walro commented Dec 3, 2020

The following spec:

context "when given a slow host" do
let(:toxiproxy) { "http_host" }
let(:url) { "http://#{ToxiproxyConfig.downstream(toxiproxy)}/" }
describe "open/read timeout" do
before { client.http_timeout = 0.1 }
around do |example|
Toxiproxy[toxiproxy].toxic(:timeout, timeout: 0).apply do
Timeout.timeout(example_timeout) do
example.run
end
end
end
it "should raise exception" do
expect { subject }
.to raise_error(Twingly::HTTP::ConnectionError)
end
end
end
has way too nice expect. Consider the following change:

diff --git a/spec/lib/twingly/http_spec.rb b/spec/lib/twingly/http_spec.rb
index 765efad..184dc42 100644
--- a/spec/lib/twingly/http_spec.rb
+++ b/spec/lib/twingly/http_spec.rb
@@ -423,9 +423,9 @@ RSpec.describe Twingly::HTTP::Client do
         WebMock.disable_net_connect!
       end
 
-      context "when given a slow host" do
+      fcontext "when given a slow host" do
         let(:toxiproxy) { "http_host" }
-        let(:url)       { "http://#{ToxiproxyConfig.downstream(toxiproxy)}/" }
+        let(:url)       { "http://thisdoesnotevenexist.com/" }
 
         describe "open/read timeout" do
           before { client.http_timeout = 0.1 }

It passes the tests, which it should not :) I guess the best solution is to introduce a Timeout error which the specs can expect rather than the general error.

@walro walro added the enhancement New feature or request label Dec 3, 2020
@walro
Copy link
Contributor Author

walro commented Dec 3, 2020

I guess the best solution is to introduce a Timeout error which the specs can expect rather than the general error.

E.g. split the following rescue in two and re-raise accordingly:

rescue *(@retryable_exceptions + TIMEOUT_EXCEPTIONS)
raise ConnectionError

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

No branches or pull requests

1 participant