httprb / http

HTTP (The Gem! a.k.a. http.rb) - a fast Ruby HTTP client with a chainable API, streaming support, and timeouts
MIT License
3.01k stars 321 forks source link

Explicitly require cgi in client_spec.rb #719

Closed matoro closed 2 years ago

matoro commented 2 years ago

I am observing the same behavior in this test as documented in https://github.com/pact-foundation/pact-support/pull/29; applying the same fix also fixes it here.

ixti commented 2 years ago

@matoro which ruby do you experience it with?

matoro commented 2 years ago

@matoro which ruby do you experience it with?

Hi @ixti I tested and observed the behavior with all of the following Ruby versions:

Ruby 2.6.10 Ruby 2.7.6 Ruby 3.0.4 Ruby 3.1.2

ixti commented 2 years ago

@matoro Which platform? I can't reproduce :((

ixti commented 2 years ago

Don't get me wrong. I'm not against merging this – but I'd like to understand the origin of the requirement. Probably we should better add this require into http/client.rb instead.

tarcieri commented 2 years ago

Probably we should better add this require into http/client.rb instead.

You did that at one point: https://github.com/httprb/http/commit/6a2a9d22902572d672dfc7c7b250d022364c8e01

Not sure what became of that.

Edit: aah, it was removed in a931f50525c02d41e6f8640b2c616ca1c6ec489b

matoro commented 2 years ago

I totally understand. I'm working on packaging http 5.x for Gentoo, so this is being done using the build sandbox created by Portage, specifically ruby-fakegem.eclass. And I'm packaging it to solve issues with http 4.x on sparc, so this is actually on a sparc machine, but I'm almost certain that the issue is platform-independent. Here is the full failure information without this change:

 * Running test phase for ruby31 ...
/usr/lib64/ruby/gems/3.1.0/gems/certificate_authority-1.0.0/lib/certificate_authority/key_material.rb:74: warning: method redefined; discarding old private_key
/usr/lib64/ruby/gems/3.1.0/gems/certificate_authority-1.0.0/lib/certificate_authority/key_material.rb:78: warning: method redefined; discarding old public_key
/usr/lib64/ruby/gems/3.1.0/gems/certificate_authority-1.0.0/lib/certificate_authority/key_material.rb:112: warning: method redefined; discarding old public_key
/usr/lib64/ruby/gems/3.1.0/gems/certificate_authority-1.0.0/lib/certificate_authority/extensions.rb:62: warning: method redefined; discarding old path_len=
/usr/lib64/ruby/gems/3.1.0/gems/certificate_authority-1.0.0/lib/certificate_authority/extensions.rb:425: warning: method redefined; discarding old uris=
/usr/lib64/ruby/gems/3.1.0/gems/certificate_authority-1.0.0/lib/certificate_authority/extensions.rb:430: warning: method redefined; discarding old dns_names=
/usr/lib64/ruby/gems/3.1.0/gems/certificate_authority-1.0.0/lib/certificate_authority/extensions.rb:435: warning: method redefined; discarding old ips=
/usr/lib64/ruby/gems/3.1.0/gems/certificate_authority-1.0.0/lib/certificate_authority/extensions.rb:440: warning: method redefined; discarding old emails=
/usr/lib64/ruby/gems/3.1.0/gems/certificate_authority-1.0.0/lib/certificate_authority/ocsp_handler.rb:116: warning: assigned but unused variable - certificate
Run options: include {:focus=>true}

All examples were filtered out; ignoring {:focus=>true}

Randomized with seed 35331
...........................................................................................F.FF........................*........................................................................
...../var/tmp/portage/dev-ruby/http-5.1.0/work/ruby31/http-5.1.0/spec/lib/http/options/headers_spec.rb:17: warning: Passing only keyword arguments to Struct#initialize will behave differently
from Ruby 3.2. Please use a Hash literal like .new({k: v}) instead of .new(k: v).
................................................................................................................................................................................................
................................................................................................................................................................................................
...................................................................................................................................................F............................................
....................................

Pending: (Failures listed here are expected and do not affect your suite's status)

  1) HTTP::Client#perform when uses chunked transfer encoding with broken body (too early closed connection) raises HTTP::ConnectionError
     # Temporarily skipped with xit
     # ./spec/lib/http/client_spec.rb:547

Failures:

  1) HTTP::Client parsing params does not corrupts index-less arrays
     Failure/Error: expect(CGI.parse(opts[:uri].query)).to eq "a[]" => %w[b c], "d" => %w[e]

     NoMethodError:
       undefined method `parse' for CGI:Class

               expect(CGI.parse(opts[:uri].query)).to eq "a[]" => %w[b c], "d" => %w[e]
                         ^^^^^^
     # ./spec/lib/http/client_spec.rb:190:in `block (4 levels) in <top (required)>'
     # ./lib/http/client.rb:46:in `build_request'
     # ./lib/http/client.rb:30:in `request'
     # ./lib/http/chainable.rb:20:in `get'
     # ./spec/lib/http/client_spec.rb:193:in `block (3 levels) in <top (required)>'

  2) HTTP::Client parsing params accepts params within the provided URL
     Failure/Error: expect(CGI.parse(opts[:uri].query)).to eq("foo" => %w[bar])

     NoMethodError:
       undefined method `parse' for CGI:Class

               expect(CGI.parse(opts[:uri].query)).to eq("foo" => %w[bar])
                         ^^^^^^
     # ./spec/lib/http/client_spec.rb:158:in `block (4 levels) in <top (required)>'
     # ./lib/http/client.rb:46:in `build_request'
     # ./lib/http/client.rb:30:in `request'
     # ./lib/http/chainable.rb:20:in `get'
     # ./spec/lib/http/client_spec.rb:161:in `block (3 levels) in <top (required)>'

  3) HTTP::Client parsing params combines GET params from the URI with the passed in params
     Failure/Error: expect(CGI.parse(opts[:uri].query)).to eq("foo" => %w[bar], "baz" => %w[quux])

     NoMethodError:
       undefined method `parse' for CGI:Class

               expect(CGI.parse(opts[:uri].query)).to eq("foo" => %w[bar], "baz" => %w[quux])
                         ^^^^^^
     # ./spec/lib/http/client_spec.rb:166:in `block (4 levels) in <top (required)>'
     # ./lib/http/client.rb:46:in `build_request'
     # ./lib/http/client.rb:30:in `request'
     # ./lib/http/chainable.rb:20:in `get'
     # ./spec/lib/http/client_spec.rb:169:in `block (3 levels) in <top (required)>'

  4) HTTP getting resources with query string parameters in the URI and opts hash includes both
     Failure/Error: expect(response.to_s).to match(/More Params!/)

       expected "<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.0//EN\">\n<HTML>\n  <HEAD><TITLE>Internal Server Error</T...EBrick/
1.7.0 (Ruby/3.1.2/2022-04-12) at\n     127.0.0.1:37407\n    </ADDRESS>\n  </BODY>\n</HTML>\n" to match /More Params!/
       Diff:
       @@ -1,16 +1,31 @@
       -/More Params!/
       +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN">
       +<HTML>
       +  <HEAD><TITLE>Internal Server Error</TITLE></HEAD>
       +  <BODY>
       +    <H1>Internal Server Error</H1>
       +    undefined method `parse' for CGI:Class
       +
       +      params = CGI.parse req.query_string
       +                  ^^^^^^
       +    <HR>
       +    <ADDRESS>
       +     WEBrick/1.7.0 (Ruby/3.1.2/2022-04-12) at
       +     127.0.0.1:37407
       +    </ADDRESS>
       +  </BODY>
       +</HTML>

     # ./spec/lib/http_spec.rb:40:in `block (4 levels) in <top (required)>'

Top 10 slowest examples (11.83 seconds, 79.7% of total time):
  HTTP::Client with a global timeout it resets state when reusing connections does not timeout
    4.02 seconds ./spec/support/http_handling_shared.rb:93
  HTTP.nodelay sets TCP_NODELAY on the underlying socket
    3.15 seconds ./spec/lib/http_spec.rb:386
  HTTP::Client with a per operation timeout read of 2.5 does not time out
    2.02 seconds ./spec/support/http_handling_shared.rb:55
  HTTP::Client with a global timeout errors if reading takes too long
    1.01 seconds ./spec/support/http_handling_shared.rb:83
  HTTP::Client with a global timeout errors if connecting takes too long
    1.01 seconds ./spec/support/http_handling_shared.rb:75
  HTTP::Client#request Feature is given a chance to handle an error
    0.20768 seconds ./spec/lib/http/client_spec.rb:368
  HTTP.cookies properly works with cookie jars from response
    0.12302 seconds ./spec/lib/http_spec.rb:344
  HTTP::Connection#readpartial reads data in parts
    0.10734 seconds ./spec/lib/http/connection_spec.rb:57
  HTTP.cookies properly merges cookies
    0.09541 seconds ./spec/lib/http_spec.rb:351
  HTTP getting resources with a large request body with `.timeout({:read=>2, :write=>2, :connect=>2})` writes the whole body
    0.08777 seconds ./spec/lib/http_spec.rb:64

Top 10 slowest example groups:
  HTTP::Client
    0.18696 seconds average (8.79 seconds / 47 examples) ./spec/lib/http/client_spec.rb:9
  HTTP::Connection
    0.07799 seconds average (0.15597 seconds / 2 examples) ./spec/lib/http/connection_spec.rb:3
  HTTP
    0.07613 seconds average (4.19 seconds / 55 examples) ./spec/lib/http_spec.rb:9
  HTTP::Options merge
    0.0102 seconds average (0.03059 seconds / 3 examples) ./spec/lib/http/options/merge_spec.rb:3
  HTTP::Features::AutoDeflate
    0.00949 seconds average (0.08539 seconds / 9 examples) ./spec/lib/http/features/auto_deflate_spec.rb:3
  HTTP::Redirector
    0.00624 seconds average (0.24972 seconds / 40 examples) ./spec/lib/http/redirector_spec.rb:3
  HTTP::Request
    0.00383 seconds average (0.17216 seconds / 45 examples) ./spec/lib/http/request_spec.rb:4
  HTTP::Response
    0.00366 seconds average (0.1098 seconds / 30 examples) ./spec/lib/http/response_spec.rb:3
  HTTP::Features::Instrumentation
    0.00363 seconds average (0.00727 seconds / 2 examples) ./spec/lib/http/features/instrumentation_spec.rb:3
  HTTP::Features::AutoInflate
    0.00352 seconds average (0.02465 seconds / 7 examples) ./spec/lib/http/features/auto_inflate_spec.rb:3

Finished in 14.84 seconds (files took 3.34 seconds to load)
809 examples, 4 failures, 1 pending

Failed examples:

rspec ./spec/lib/http/client_spec.rb:188 # HTTP::Client parsing params does not corrupts index-less arrays
rspec ./spec/lib/http/client_spec.rb:156 # HTTP::Client parsing params accepts params within the provided URL
rspec ./spec/lib/http/client_spec.rb:164 # HTTP::Client parsing params combines GET params from the URI with the passed in para
ms
rspec ./spec/lib/http_spec.rb:38 # HTTP getting resources with query string parameters in the URI and opts hash includes both

Randomized with seed 35331
ixti commented 2 years ago

Oh. I got it now. @tarcieri you'r right we've removed it from lib, but specs still use it. @matoro I'll add one more require and will merge your PR.

ixti commented 2 years ago

Fixed: https://github.com/httprb/http/commit/85f7ddbe7877ffd128ecd4f71bdada0585d95cb9