Merge pull request #378 from chadlwilson/1-2-fix-flaky-tests

1.2: [tests] fix flaky tests

Author: chadlwilson

.bundle/config

@@ -0,0 +1,2 @@
+---
+BUNDLE_VERSION: "system"

examples/camping/.bundle/config

@@ -0,0 +1,2 @@
+---
+BUNDLE_VERSION: "system"

examples/rails8/.bundle/config

@@ -0,0 +1,2 @@
+---
+BUNDLE_VERSION: "system"

examples/sinatra/.bundle/config

@@ -0,0 +1,2 @@
+---
+BUNDLE_VERSION: "system"

gemfiles/.bundle/config

@@ -1,2 +1,2 @@
 ---
-BUNDLE_RETRY: "1"
+BUNDLE_VERSION: "system"

src/main/java/org/jruby/rack/RackApplicationFactoryDecorator.java

@@ -137,8 +137,8 @@ public void destroy() {
     public RackApplication getApplication() throws RackException {
         final RuntimeException error = getInitError();
         if ( error != null ) {
-            log(DEBUG, "due a previous initialization failure application instance can not be returned");
-            throw error; // this is better - we shall never return null here ...
+            log(DEBUG, "due to a previous initialization failure application instance can not be returned");
+            throw error;
         }
         return getApplicationImpl();
     }

src/spec/ruby/jruby/rack/booter_spec.rb

@@ -299,17 +299,17 @@
         @runtime.evalScriptlet("load 'jruby/rack/boot/rack.rb'")
 
         # booter got setup :
-        should_not_eval_as_nil "defined?(JRuby::Rack.booter)"
-        should_not_eval_as_nil "JRuby::Rack.booter"
+        should_eval_as_not_nil "defined?(JRuby::Rack.booter)"
+        should_eval_as_not_nil "JRuby::Rack.booter"
         should_eval_as_eql_to "JRuby::Rack.booter.class.name", 'JRuby::Rack::Booter'
 
         # Booter.boot! run :
-        should_not_eval_as_nil "ENV['RACK_ENV']"
+        should_eval_as_not_nil "ENV['RACK_ENV']"
         # rack got required :
-        should_not_eval_as_nil "defined?(Rack::VERSION)"
-        should_not_eval_as_nil "defined?(Rack.release)"
+        should_eval_as_not_nil "defined?(Rack::VERSION)"
+        should_eval_as_not_nil "defined?(Rack.release)"
         # check if it got loaded correctly :
-        should_not_eval_as_nil "Rack::Request.new({}) rescue nil"
+        should_eval_as_not_nil "Rack::Request.new({}) rescue nil"
       end
 
     end
@@ -346,13 +346,13 @@
         @runtime.evalScriptlet("load 'jruby/rack/boot/rails.rb'")
 
         # booter got setup :
-        should_not_eval_as_nil "defined?(JRuby::Rack.booter)"
-        should_not_eval_as_nil "JRuby::Rack.booter"
+        should_eval_as_not_nil "defined?(JRuby::Rack.booter)"
+        should_eval_as_not_nil "JRuby::Rack.booter"
         should_eval_as_eql_to "JRuby::Rack.booter.class.name", 'JRuby::Rack::RailsBooter'
 
         # Booter.boot! run :
-        should_not_eval_as_nil "ENV['RACK_ENV']"
-        should_not_eval_as_nil "ENV['RAILS_ENV']"
+        should_eval_as_not_nil "ENV['RACK_ENV']"
+        should_eval_as_not_nil "ENV['RAILS_ENV']"
 
         # rack not yet required (let bundler decide which rack version to load) :
         should_eval_as_nil "defined?(Rack::VERSION)"

src/spec/ruby/jruby/rack/rails_booter_spec.rb

@@ -129,10 +129,6 @@
       end
     end
 
-    after :all do
-      $servlet_context = nil
-    end
-
     it "should have loaded the railtie" do
       expect(defined?(JRuby::Rack::Railtie)).not_to be nil
     end

src/spec/ruby/rack/application_spec.rb

@@ -104,6 +104,8 @@ def it_should_rewind_body
 
   before :each do
     @app_factory = DefaultRackApplicationFactory.new
+    reset_booter
+    reset_servlet_context_global
   end
 
   it "should receive a rackup script via the 'rackup' parameter" do
@@ -161,11 +163,6 @@ def it_should_rewind_body
     expect(input_stream.getMaximumBufferSize).to eq 420
   end
 
-  before do
-    reset_booter
-    JRuby::Rack.context = $servlet_context = nil
-  end
-
   it "should init and create application object without a rackup script" do
     $servlet_context = @servlet_context
     # NOTE: a workaround to be able to mock it :
@@ -303,8 +300,8 @@ def newRuntime()
 
       it "creates a new Ruby runtime with the jruby-rack environment pre-loaded" do
         @runtime = app_factory.newRuntime
-        should_not_eval_as_nil "defined?(::Rack)"
-        should_not_eval_as_nil "defined?(::Rack::Handler::Servlet)"
+        should_eval_as_not_nil "defined?(::Rack)"
+        should_eval_as_not_nil "defined?(::Rack::Handler::Servlet)"
         should_eval_as_nil "defined?(Rack::Handler::Bogus)"
       end
 
@@ -333,14 +330,14 @@ def newRuntime()
         app_factory.checkAndSetRackVersion(@runtime)
         @runtime.evalScriptlet "require 'rack'"
 
-        should_not_eval_as_nil "defined?(Bundler)"
+        should_eval_as_not_nil "defined?(Bundler)"
         should_eval_as_eql_to "Rack.release", '2.2.0'
         should_eval_as_eql_to "Gem.loaded_specs['rack'].version.to_s", '2.2.0'
       end
 
       it "initializes the $servlet_context global variable" do
         @runtime = app_factory.new_runtime
-        should_not_eval_as_nil "defined?($servlet_context)"
+        expect(@runtime.evalScriptlet("defined?($servlet_context)")).to be_truthy
       end
 
       it "clears environment variables if the configuration ignores the environment" do
@@ -534,7 +531,7 @@ def reset_config
 
 describe org.jruby.rack.rails.RailsRackApplicationFactory do
 
-  java_import org.jruby.rack.rails.RailsRackApplicationFactory
+  require 'jruby/rack/rails_booter'
 
   before :each do
     @app_factory = RailsRackApplicationFactory.new
@@ -575,8 +572,20 @@ def createRackServletWrapper(runtime, rackup, filename)
 
 describe org.jruby.rack.PoolingRackApplicationFactory do
 
+  # Workaround rspec mocks/proxies not being thread-safe which causes occasional failures
+  class Synchronized
+    def initialize(obj)
+      @delegate = obj
+      @lock = Mutex.new
+    end
+
+    def method_missing(name, *args, &block)
+      @lock.synchronize { @delegate.send(name, *args, &block) }
+    end
+  end
+
   before :each do
-    @factory = double "factory"
+    @factory = Synchronized.new(double("factory").as_null_object)
     @pooling_factory = org.jruby.rack.PoolingRackApplicationFactory.new @factory
     @pooling_factory.context = @rack_context
   end
@@ -626,7 +635,7 @@ def createRackServletWrapper(runtime, rackup, filename)
   it "creates applications during initialization according to the jruby.min.runtimes context parameter" do
     allow(@factory).to receive(:init)
     allow(@factory).to receive(:newApplication) do
-      app = double "app"
+      app = Synchronized.new(double("app").as_null_object)
       expect(app).to receive(:init)
       app
     end
@@ -659,7 +668,7 @@ def createRackServletWrapper(runtime, rackup, filename)
   it "forces the maximum size to be greater or equal to the initial size" do
     allow(@factory).to receive(:init)
     allow(@factory).to receive(:newApplication) do
-      app = double "app"
+      app = Synchronized.new(double("app").as_null_object)
       expect(app).to receive(:init)
       app
     end
@@ -673,17 +682,17 @@ def createRackServletWrapper(runtime, rackup, filename)
   end
 
   it "retrieves the error application from the delegate factory" do
-    app = double("app")
+    app = double "app"
     expect(@factory).to receive(:getErrorApplication).and_return app
     expect(@pooling_factory.getErrorApplication).to eq app
   end
 
   it "waits till initial runtimes get initialized (with wait set to true)" do
     allow(@factory).to receive(:init)
     allow(@factory).to receive(:newApplication) do
-      app = double "app"
+      app = Synchronized.new(double("app").as_null_object)
       allow(app).to receive(:init) do
-        sleep(0.10)
+        sleep(0.05)
       end
       app
     end
@@ -697,15 +706,16 @@ def createRackServletWrapper(runtime, rackup, filename)
 
   it "throws an exception from getApplication when an app failed to initialize " +
      "(even when only a single application initialization fails)" do
+    app_init_secs = 0.05
     allow(@factory).to receive(:init)
     app_count = java.util.concurrent.atomic.AtomicInteger.new(0)
     allow(@factory).to receive(:newApplication) do
-      app = double "app"
+      app = Synchronized.new(double("app").as_null_object)
       allow(app).to receive(:init) do
         if app_count.addAndGet(1) == 2
           raise org.jruby.rack.RackInitializationException.new('failed app init')
         end
-        sleep(0.05)
+        sleep(app_init_secs)
       end
       app
     end
@@ -719,7 +729,7 @@ def createRackServletWrapper(runtime, rackup, filename)
     rescue org.jruby.rack.RackInitializationException
       # ignore - sometimes initialization happens fast enough that the init error is thrown already
     end
-    sleep(0.20)
+    sleep(num_runtimes * app_init_secs + 0.07) # sleep with a buffer
 
     failed = 0
     num_runtimes.times do
@@ -735,28 +745,29 @@ def createRackServletWrapper(runtime, rackup, filename)
   end
 
   it "wait until pool is filled when invoking getApplication (with wait set to false)" do
+    app_init_secs = 0.2
     allow(@factory).to receive(:init)
     allow(@factory).to receive(:newApplication) do
-      app = double "app"
-      allow(app).to receive(:init) { sleep(0.2) }
+      app = Synchronized.new(double("app").as_null_object)
+      allow(app).to receive(:init) { sleep(app_init_secs) }
       app
     end
     allow(@rack_config).to receive(:getBooleanProperty).with("jruby.runtime.init.wait").and_return false
     expect(@rack_config).to receive(:getInitialRuntimes).and_return 3
     expect(@rack_config).to receive(:getMaximumRuntimes).and_return 4
 
     @pooling_factory.init(@rack_context)
-    millis = java.lang.System.currentTimeMillis
+    start = java.lang.System.currentTimeMillis
     expect(@pooling_factory.getApplication).not_to be nil
-    millis = java.lang.System.currentTimeMillis - millis
-    expect(millis).to be >= 150 # getApplication waited ~ 0.2 secs
+    expect(java.lang.System.currentTimeMillis - start).to be_within(70).of(app_init_secs * 1000) # getApplication waited ~ sleep time
   end
 
   it "waits acquire timeout till an application is available from the pool (than raises)" do
+    app_init_secs = 0.2
     allow(@factory).to receive(:init)
     expect(@factory).to receive(:newApplication).twice do
-      app = double "app"
-      expect(app).to receive(:init) { sleep(0.2) }
+      app = Synchronized.new(double("app").as_null_object)
+      expect(app).to receive(:init) { sleep(app_init_secs) }
       app
     end
     allow(@rack_config).to receive(:getBooleanProperty).with("jruby.runtime.init.wait").and_return false
@@ -765,86 +776,90 @@ def createRackServletWrapper(runtime, rackup, filename)
 
     @pooling_factory.init(@rack_context)
     @pooling_factory.acquire_timeout = 1.to_java # second
-    millis = java.lang.System.currentTimeMillis
+    start = java.lang.System.currentTimeMillis
     expect(@pooling_factory.getApplication).not_to be nil
-    millis = java.lang.System.currentTimeMillis - millis
-    expect(millis).to be >= 150 # getApplication waited ~ 0.2 secs
+    expect(java.lang.System.currentTimeMillis - start).to be_within(70).of(app_init_secs * 1000)
 
     app2 = @pooling_factory.getApplication # now the pool is empty
-
-    @pooling_factory.acquire_timeout = 0.1.to_java # second
-    millis = java.lang.System.currentTimeMillis
+    timeout_secs = 0.1
+    @pooling_factory.acquire_timeout = (timeout_secs).to_java
+    start = java.lang.System.currentTimeMillis
     expect { @pooling_factory.getApplication }.to raise_error(org.jruby.rack.AcquireTimeoutException)
-    millis = java.lang.System.currentTimeMillis - millis
-    expect(millis).to be >= 90 # waited about ~ 0.1 secs
+    expect(java.lang.System.currentTimeMillis - start).to be_within(20).of(timeout_secs * 1000)
 
     @pooling_factory.finishedWithApplication(app2) # gets back to the pool
     expect(@pooling_factory.getApplication).to eq app2
   end
 
   it "gets and initializes new applications until maximum allows to create more" do
+    app_init_secs = 0.1
     allow(@factory).to receive(:init)
     expect(@factory).to receive(:newApplication).twice do
-      app = double "app (new)"
-      expect(app).to receive(:init) { sleep(0.1) }
+      app = Synchronized.new(double("app (new)").as_null_object)
+      expect(app).to receive(:init) { sleep(app_init_secs) }
       app
     end
     allow(@rack_config).to receive(:getBooleanProperty).with("jruby.runtime.init.wait").and_return false
     allow(@rack_config).to receive(:getInitialRuntimes).and_return 2
     allow(@rack_config).to receive(:getMaximumRuntimes).and_return 4
 
+    timeout_secs = 0.1
     @pooling_factory.init(@rack_context)
-    @pooling_factory.acquire_timeout = 0.10.to_java # second
+    @pooling_factory.acquire_timeout = (timeout_secs).to_java # second
 
     2.times { expect(@pooling_factory.getApplication).not_to be nil }
 
+    app_get_secs = 0.15
     expect(@factory).to receive(:getApplication).twice do
-      app = double "app (get)"; sleep(0.15); app
+      app = Synchronized.new(double("app (get)").as_null_object)
+      sleep(app_get_secs)
+      app
     end
 
-    millis = java.lang.System.currentTimeMillis
+    start = java.lang.System.currentTimeMillis
     2.times { expect(@pooling_factory.getApplication).not_to be nil }
-    millis = java.lang.System.currentTimeMillis - millis
-    expect(millis).to be >= 300 # waited about 2 x 0.15 secs
+    expect(java.lang.System.currentTimeMillis - start).to be_within(70).of(2 * app_get_secs * 1000)
 
-    millis = java.lang.System.currentTimeMillis
+    start = java.lang.System.currentTimeMillis
     expect {
       @pooling_factory.getApplication
     }.to raise_error(org.jruby.rack.AcquireTimeoutException)
-    millis = java.lang.System.currentTimeMillis - millis
-    expect(millis).to be >= 90 # waited about ~ 0.10 secs
+    expect(java.lang.System.currentTimeMillis - start).to be_within(20).of(timeout_secs * 1000)
   end
 
-  it "initializes initial runtimes in paralel (with wait set to false)" do
+  it "initializes initial runtimes in parallel (with wait set to false)" do
+    app_init_secs = 0.15
     allow(@factory).to receive(:init)
     allow(@factory).to receive(:newApplication) do
-      app = double "app"
-      allow(app).to receive(:init) do
-        sleep(0.15)
-      end
+      app = Synchronized.new(double("app").as_null_object)
+      allow(app).to receive(:init) { sleep(app_init_secs) }
       app
     end
+
+    init_threads = 4
+    init_runtimes = 6
     allow(@rack_config).to receive(:getBooleanProperty).with("jruby.runtime.init.wait").and_return false
-    allow(@rack_config).to receive(:getInitialRuntimes).and_return 6
+    allow(@rack_config).to receive(:getRuntimeInitThreads).and_return init_threads
+    allow(@rack_config).to receive(:getInitialRuntimes).and_return init_runtimes
     allow(@rack_config).to receive(:getMaximumRuntimes).and_return 8
 
+    expected_initial_init_time = 1.1 * (init_runtimes.to_f / init_threads.to_f).ceil * app_init_secs # 10% margin
     @pooling_factory.init(@rack_context)
-    sleep(0.10)
-    expect(@pooling_factory.getApplicationPool.size).to be < 6
-    sleep(0.9)
-    expect(@pooling_factory.getApplicationPool.size).to be >= 6
+    sleep(app_init_secs) # wait for at some (but not possibly all) to finish
+    expect(@pooling_factory.getApplicationPool.size).to be < init_runtimes
+    sleep(expected_initial_init_time - app_init_secs) # remaining time
+    expect(@pooling_factory.getApplicationPool.size).to be >= init_runtimes
 
     expect(@pooling_factory.getManagedApplications).to_not be_empty
-    expect(@pooling_factory.getManagedApplications.size).to eql 6
+    expect(@pooling_factory.getManagedApplications.size).to eql init_runtimes
   end
 
   it "throws from init when application initialization in thread failed" do
+    app_init_secs = 0.05
     allow(@factory).to receive(:init)
     allow(@factory).to receive(:newApplication) do
-      app = double "app"
-      allow(app).to receive(:init) do
-        sleep(0.05); raise "app.init raising"
-      end
+      app = Synchronized.new(double("app").as_null_object)
+      allow(app).to receive(:init) { sleep(app_init_secs); raise "app.init raising" }
       app
     end
     allow(@rack_config).to receive(:getInitialRuntimes).and_return 2
@@ -863,9 +878,6 @@ def createRackServletWrapper(runtime, rackup, filename)
 
     expect { @pooling_factory.init(@rack_context) }.to raise_error org.jruby.rack.RackInitializationException
     expect(raise_error_logged).to eql 1 # logs same init exception once
-
-    # NOTE: seems it's not such a good idea to return empty on init error
-    #     expect(@pooling_factory.getManagedApplications).to be_empty
   end
 
 end