Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix integration tests for Net HTTP Server. #671

Merged
merged 3 commits into from

3 participants

@patriciomacadden

This fixes #670.

Unfortunately, seems like parslet (a net-http-server dependency) is broken, which causes starts the correct server test to fail.

The problem is that when running:

RACK_ENV=development exec "/Users/patricio/.rbenv/versions/1.9.3-p392/bin/ruby" -w -I "/Users/patricio/Code/github/sinatra/lib" "/Users/patricio/Code/github/sinatra/test/integration/app.rb" -s HTTP -o 127.0.0.1 -p 5002 -e development 2>&1

(generated by https://github.com/sinatra/sinatra/blob/master/test/integration_helper.rb#L96)

parslet crashes:

loading
starting
/Users/patricio/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/net-http-server-0.2.2/lib/net/http/server/daemon.rb:109: warning: assigned but unused variable - error
/Users/patricio/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/parslet-1.5.0/lib/parslet/source/line_cache.rb:42: warning: `-' after local variable is interpreted as binary operator
/Users/patricio/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/parslet-1.5.0/lib/parslet/source/line_cache.rb:42: warning: even though it seems like unary operator
/Users/patricio/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/parslet-1.5.0/lib/parslet/atoms/base.rb:83: warning: assigned but unused variable - value
/Users/patricio/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/parslet-1.5.0/lib/parslet/atoms/lookahead.rb:27: warning: assigned but unused variable - value
/Users/patricio/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/parslet-1.5.0/lib/parslet/error_reporter/deepest.rb:62: warning: assigned but unused variable - rank
/Users/patricio/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/net-http-server-0.2.2/lib/net/http/server/chunked_stream.rb:51: warning: assigned but unused variable - chunk_extension
@rkh rkh commented on the diff
test/integration_test.rb
@@ -17,7 +17,13 @@ class IntegrationTest < Test::Unit::TestCase
random = "%064x" % Kernel.rand(2**256-1)
server.get "/ping?x=#{random}"
count = server.log.scan("GET /ping?x=#{random}").count
- server.webrick? ? assert(count > 0) : assert_equal(1, count)
+ if server.net_http_server?
+ assert_equal 0, count
@rkh Owner
rkh added a note

This means logging is not working with net-http-server, right?

Exactly. Even if you run a simple app like:

require 'sinatra'

get('/') { "Hello world" }
ruby t.rb -s HTTP

You get no output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@patriciomacadden

Apparently net-http-server is not an active project. Last commit was 6 months ago.

@rkh
Owner

paging @postmodern

@postmodern

After installing net-http-server and pulling sinatra master, test/integration_test.rb can't find net-http-server.

$ ruby -Ilib test/integration_test.rb
thin is not installed, skipping integration tests
http is not installed, skipping integration tests
Run options: 

# Running tests:

................

Finished tests in 2.516450s, 6.3582 tests/s, 6.3582 assertions/s.

16 tests, 16 assertions, 0 failures, 0 errors, 0 skips
@patriciomacadden

Yes, that's why I'm creating this PR. The integration tests couldn't load net-http-server correctly (was requiring http instead of net/http/server).

If you pull this branch and run the tests, parslet will crash:

loading
starting
/Users/patricio/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/net-http-server-0.2.2/lib/net/http/server/daemon.rb:109: warning: assigned but unused variable - error
/Users/patricio/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/parslet-1.5.0/lib/parslet/source/line_cache.rb:42: warning: `-' after local variable is interpreted as binary operator
/Users/patricio/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/parslet-1.5.0/lib/parslet/source/line_cache.rb:42: warning: even though it seems like unary operator
/Users/patricio/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/parslet-1.5.0/lib/parslet/atoms/base.rb:83: warning: assigned but unused variable - value
/Users/patricio/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/parslet-1.5.0/lib/parslet/atoms/lookahead.rb:27: warning: assigned but unused variable - value
/Users/patricio/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/parslet-1.5.0/lib/parslet/error_reporter/deepest.rb:62: warning: assigned but unused variable - rank
/Users/patricio/.rbenv/versions/1.9.3-p392/lib/ruby/gems/1.9.1/gems/net-http-server-0.2.2/lib/net/http/server/chunked_stream.rb:51: warning: assigned but unused variable - chunk_extension

Thanks for coming in!

@postmodern

OK, grabbed your branch. The warnings from parslet are due to the -w option being used; which we could omit?

Also, Net::HTTP::Server::Daemon defaults the log stream to $stderr. Is this acceptable behaviour?

@patriciomacadden

Oh! I see, I tried before without the -w option but the test failed too. Maybe we could log to $stdout instead of $stderr in the tests so all tests mean the same.

Thanks, @postmodern! I'll update the commit.

@patriciomacadden

I can't change the log stream to $stdout. The test generates the command (https://github.com/sinatra/sinatra/blob/master/test/integration_helper.rb#L96):

RACK_ENV=development exec "/Users/patricio/.rbenv/versions/1.9.3-p392/bin/ruby" -I "/Users/patricio/Code/github/sinatra/lib" "/Users/patricio/Code/github/sinatra/test/integration/app.rb" -s HTTP -o 127.0.0.1 -p 5002 -e development 2>&1

All options are parsed by OptionParser (https://github.com/sinatra/sinatra/blob/master/lib/sinatra/main.rb#L15), and there is no option I can set to change the log stream. (Or there is?)

I think I can change starts the correct server to knowing that net-http-server logs to $stderr (like in logs once in development mode).

I'll fix that and update the PR. Please review it, @rkh

@postmodern

The command it generates includes 2>&1, so logging to stderr shouldn't be a problem. Although I'm not sure if stderr is the correct place to log messages?

@patriciomacadden

My answer to that question might be: "it depends". Depends on "what to log". In general, web servers such as apache have 2 log streams: one for errors and one for common messages (such as accesses). And when you deploy with unicorn, for instance, you have 2 log streams too.

@patriciomacadden

Oops, it's failing! I'm looking into it later.

@patriciomacadden

I think I got it. The tests under:

  • 1.8.7
  • rbx-18mode
  • rbx-19mode
  • jruby-18mode
  • jruby-19mode
  • jruby-head

are failing because in https://github.com/postmodern/net-http-server/blob/master/lib/rack/handler/http.rb#L92 the handler calls remote_address = stream.socket.remote_address.

stream is an instance of Net::HTTP::Server::Stream, and it's instance variable socket is an instance of TCPSocket. The hierarchy of TCPSocket is:

TCPSocket < IPSocket < BasicSocket

In 1.9.3 (and all vms where the test doesn't fails), remote_address is a method of BasicSocket [1], which is not present in 1.8.7 [2], rbx-18mode [3], rbx-19mode [4], jruby-18mode [5], jruby-19mode [6], and jruby-head [7]

I'll fix the tests to get this working, but please correct me if I'm wrong.

[1] http://www.ruby-doc.org/stdlib-1.9.3/libdoc/socket/rdoc/BasicSocket.html#method-i-remote_address
[2] http://www.ruby-doc.org/stdlib-1.8.7/libdoc/socket/rdoc/BasicSocket.html
[3] https://github.com/rubinius/rubinius/blob/master/lib/18/socket.rb
[4] https://github.com/rubinius/rubinius/blob/master/lib/19/socket.rb
[5] Could not find socket.rb
[6] https://github.com/jruby/jruby/blob/master/lib/ruby/1.9/socket.rb
[7] https://github.com/jruby/jruby/blob/master/lib/ruby/2.0/socket.rb (Not sure if this is jruby-head)

@patriciomacadden patriciomacadden commented on the diff
@@ -75,12 +75,9 @@ if RUBY_ENGINE != 'jruby' or not ENV['TRAVIS']
end
gem 'RedCloth' unless RUBY_ENGINE == "macruby"
gem 'puma'
-
- ## bluecloth is broken
- #gem 'bluecloth'

removed this because I added gem 'bluecloth' in a previous commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rkh
Owner

But it's still not logging anything? I'm just not sure why we're testing for that then. This test would break if it should start logging all of a sudden.

@rkh
Owner

Ah, nvm, it's checking in a different place.

@rkh rkh merged commit 9fbb92f into sinatra:master

1 check passed

Details default The Travis build passed
@patriciomacadden

It's logging, but to $stderr, which means that the output used in the tests is not useful.

Thanks for merging.

@patriciomacadden patriciomacadden deleted the patriciomacadden:fix-integration-tests branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 10, 2013
  1. @patriciomacadden
Commits on Mar 11, 2013
  1. @patriciomacadden
Commits on Mar 12, 2013
  1. @patriciomacadden
This page is out of date. Refresh to see the latest.
View
5 Gemfile
@@ -75,12 +75,9 @@ if RUBY_ENGINE != 'jruby' or not ENV['TRAVIS']
end
gem 'RedCloth' unless RUBY_ENGINE == "macruby"
gem 'puma'
-
- ## bluecloth is broken
- #gem 'bluecloth'

removed this because I added gem 'bluecloth' in a previous commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
-gem 'net-http-server'
+gem 'net-http-server' unless RUBY_VERSION == '1.8.7' || RUBY_ENGINE =~ /jruby|rbx/
platforms :ruby_18, :jruby do
gem 'json' unless RUBY_VERSION > '1.9' # is there a jruby but 1.8 only selector?
View
2  lib/sinatra/base.rb
@@ -1755,7 +1755,7 @@ class << self
set :run, false # start server via at-exit hook?
set :running, false # is the built-in server running now?
- set :server, %w[http webrick]
+ set :server, %w[HTTP webrick]
set :bind, Proc.new { development? ? 'localhost' : '0.0.0.0' }
set :port, Integer(ENV['PORT'] || 4567)
View
9 test/integration_helper.rb
@@ -86,7 +86,8 @@ def log
def installed?
return @installed unless @installed.nil?
- require server
+ s = server == 'HTTP' ? 'net/http/server' : server
+ require s
@installed = true
rescue LoadError
warn "#{server} is not installed, skipping integration tests"
@@ -102,7 +103,7 @@ def command
file, dir = RbConfig::CONFIG.values_at('ruby_install_name', 'bindir')
cmd << File.expand_path(file, dir).inspect
end
- cmd << "-w" unless thin?
+ cmd << "-w" unless thin? || net_http_server?
cmd << "-I" << File.expand_path('../../lib', __FILE__).inspect
cmd << app_file.inspect << '-s' << server << '-o' << '127.0.0.1' << '-p' << port
cmd << "-e" << environment.to_s << '2>&1'
@@ -134,6 +135,10 @@ def trinidad?
name.to_s == "trinidad"
end
+ def net_http_server?
+ name.to_s == 'HTTP'
+ end
+
def warnings
log.scan(%r[(?:\(eval|lib/sinatra).*warning:.*$])
end
View
11 test/integration_test.rb
@@ -17,7 +17,13 @@ class IntegrationTest < Test::Unit::TestCase
random = "%064x" % Kernel.rand(2**256-1)
server.get "/ping?x=#{random}"
count = server.log.scan("GET /ping?x=#{random}").count
- server.webrick? ? assert(count > 0) : assert_equal(1, count)
+ if server.net_http_server?
+ assert_equal 0, count
@rkh Owner
rkh added a note

This means logging is not working with net-http-server, right?

Exactly. Even if you run a simple app like:

require 'sinatra'

get('/') { "Hello world" }
ruby t.rb -s HTTP

You get no output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ elsif server.webrick?
+ assert(count > 0)
+ else
+ assert_equal(1, count)
+ end
end
it 'streams' do
@@ -76,7 +82,8 @@ class IntegrationTest < Test::Unit::TestCase
with\sbackup\sfrom\s#{server}
}ix
- assert_match exp, server.log
+ # because Net HTTP Server logs to $stderr by default
+ assert_match exp, server.log unless server.net_http_server?
end
it 'does not generate warnings' do
Something went wrong with that request. Please try again.