Refactor notifications checks in tests (#657)

spec/acceptance/blocking_ip_spec.rb

@@ -3,6 +3,8 @@
 require_relative "../spec_helper"
 
 describe "Blocking an IP" do
+  let(:notifications) { [] }
+
   before do
     Rack::Attack.blocklist_ip("1.2.3.4")
   end
@@ -26,21 +28,18 @@ describe "Blocking an IP" do
   end
 
   it "notifies when the request is blocked" do
-    notified = false
-    notification_type = nil
-
     ActiveSupport::Notifications.subscribe("blocklist.rack_attack") do |_name, _start, _finish, _id, payload|
-      notified = true
-      notification_type = payload[:request].env["rack.attack.match_type"]
+      notifications.push(payload)
     end
 
     get "/", {}, "REMOTE_ADDR" => "5.6.7.8"
 
-    refute notified
+    assert notifications.empty?
 
     get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
 
-    assert notified
-    assert_equal :blocklist, notification_type
+    assert_equal 1, notifications.size
+    notification = notifications.pop
+    assert_equal :blocklist, notification[:request].env["rack.attack.match_type"]
   end
 end

spec/acceptance/blocking_spec.rb

@@ -3,6 +3,8 @@
 require_relative "../spec_helper"
 
 describe "#blocklist" do
+  let(:notifications) { [] }
+
   before do
     Rack::Attack.blocklist do |request|
       request.ip == "1.2.3.4"
@@ -22,27 +24,26 @@ describe "#blocklist" do
   end
 
   it "notifies when the request is blocked" do
-    notification_matched = nil
-    notification_type = nil
-
     ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, payload|
-      notification_matched = payload[:request].env["rack.attack.matched"]
-      notification_type = payload[:request].env["rack.attack.match_type"]
+      notifications.push(payload)
     end
 
     get "/", {}, "REMOTE_ADDR" => "5.6.7.8"
 
-    assert_nil notification_matched
-    assert_nil notification_type
+    assert notifications.empty?
 
     get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
 
-    assert_nil notification_matched
-    assert_equal :blocklist, notification_type
+    assert_equal 1, notifications.size
+    notification = notifications.pop
+    assert_nil notification[:request].env["rack.attack.matched"]
+    assert_equal :blocklist, notification[:request].env["rack.attack.match_type"]
   end
 end
 
 describe "#blocklist with name" do
+  let(:notifications) { [] }
+
   before do
     Rack::Attack.blocklist("block 1.2.3.4") do |request|
       request.ip == "1.2.3.4"
@@ -62,22 +63,19 @@ describe "#blocklist with name" do
   end
 
   it "notifies when the request is blocked" do
-    notification_matched = nil
-    notification_type = nil
-
     ActiveSupport::Notifications.subscribe("blocklist.rack_attack") do |_name, _start, _finish, _id, payload|
-      notification_matched = payload[:request].env["rack.attack.matched"]
-      notification_type = payload[:request].env["rack.attack.match_type"]
+      notifications.push(payload)
     end
 
     get "/", {}, "REMOTE_ADDR" => "5.6.7.8"
 
-    assert_nil notification_matched
-    assert_nil notification_type
+    assert notifications.empty?
 
     get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
 
-    assert_equal "block 1.2.3.4", notification_matched
-    assert_equal :blocklist, notification_type
+    assert_equal 1, notifications.size
+    notification = notifications.pop
+    assert_equal "block 1.2.3.4", notification[:request].env["rack.attack.matched"]
+    assert_equal :blocklist, notification[:request].env["rack.attack.match_type"]
   end
 end

spec/acceptance/blocking_subnet_spec.rb

@@ -3,6 +3,8 @@
 require_relative "../spec_helper"
 
 describe "Blocking an IP subnet" do
+  let(:notifications) { [] }
+
   before do
     Rack::Attack.blocklist_ip("1.2.3.4/31")
   end
@@ -26,21 +28,18 @@ describe "Blocking an IP subnet" do
   end
 
   it "notifies when the request is blocked" do
-    notified = false
-    notification_type = nil
-
     ActiveSupport::Notifications.subscribe("blocklist.rack_attack") do |_name, _start, _finish, _id, payload|
-      notified = true
-      notification_type = payload[:request].env["rack.attack.match_type"]
+      notifications.push(payload)
     end
 
     get "/", {}, "REMOTE_ADDR" => "5.6.7.8"
 
-    refute notified
+    assert notifications.empty?
 
     get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
 
-    assert notified
-    assert_equal :blocklist, notification_type
+    assert_equal 1, notifications.size
+    notification = notifications.pop
+    assert_equal :blocklist, notification[:request].env["rack.attack.match_type"]
   end
 end

spec/acceptance/safelisting_ip_spec.rb

@@ -3,6 +3,8 @@
 require_relative "../spec_helper"
 
 describe "Safelist an IP" do
+  let(:notifications) { [] }
+
   before do
     Rack::Attack.blocklist("admin") do |request|
       request.path == "/admin"
@@ -42,15 +44,15 @@ describe "Safelist an IP" do
   end
 
   it "notifies when the request is safe" do
-    notification_type = nil
-
     ActiveSupport::Notifications.subscribe("safelist.rack_attack") do |_name, _start, _finish, _id, payload|
-      notification_type = payload[:request].env["rack.attack.match_type"]
+      notifications.push(payload)
     end
 
     get "/admin", {}, "REMOTE_ADDR" => "5.6.7.8"
 
     assert_equal 200, last_response.status
-    assert_equal :safelist, notification_type
+    assert_equal 1, notifications.size
+    notification = notifications.pop
+    assert_equal :safelist, notification[:request].env["rack.attack.match_type"]
   end
 end

spec/acceptance/safelisting_spec.rb

@@ -3,6 +3,8 @@
 require_relative "../spec_helper"
 
 describe "#safelist" do
+  let(:notifications) { [] }
+
   before do
     Rack::Attack.blocklist do |request|
       request.ip == "1.2.3.4"
@@ -38,23 +40,23 @@ describe "#safelist" do
   end
 
   it "notifies when the request is safe" do
-    notification_matched = nil
-    notification_type = nil
-
     ActiveSupport::Notifications.subscribe("rack.attack") do |_name, _start, _finish, _id, payload|
-      notification_matched = payload[:request].env["rack.attack.matched"]
-      notification_type = payload[:request].env["rack.attack.match_type"]
+      notifications.push(payload)
     end
 
     get "/safe_space", {}, "REMOTE_ADDR" => "1.2.3.4"
 
     assert_equal 200, last_response.status
-    assert_nil notification_matched
-    assert_equal :safelist, notification_type
+    assert_equal 1, notifications.size
+    notification = notifications.pop
+    assert_nil notification[:request].env["rack.attack.matched"]
+    assert_equal :safelist, notification[:request].env["rack.attack.match_type"]
   end
 end
 
 describe "#safelist with name" do
+  let(:notifications) { [] }
+
   before do
     Rack::Attack.blocklist("block 1.2.3.4") do |request|
       request.ip == "1.2.3.4"
@@ -90,18 +92,16 @@ describe "#safelist with name" do
   end
 
   it "notifies when the request is safe" do
-    notification_matched = nil
-    notification_type = nil
-
     ActiveSupport::Notifications.subscribe("safelist.rack_attack") do |_name, _start, _finish, _id, payload|
-      notification_matched = payload[:request].env["rack.attack.matched"]
-      notification_type = payload[:request].env["rack.attack.match_type"]
+      notifications.push(payload)
     end
 
     get "/safe_space", {}, "REMOTE_ADDR" => "1.2.3.4"
 
     assert_equal 200, last_response.status
-    assert_equal "safe path", notification_matched
-    assert_equal :safelist, notification_type
+    assert_equal 1, notifications.size
+    notification = notifications.pop
+    assert_equal "safe path", notification[:request].env["rack.attack.matched"]
+    assert_equal :safelist, notification[:request].env["rack.attack.match_type"]
   end
 end

spec/acceptance/safelisting_subnet_spec.rb

@@ -3,6 +3,8 @@
 require_relative "../spec_helper"
 
 describe "Safelisting an IP subnet" do
+  let(:notifications) { [] }
+
   before do
     Rack::Attack.blocklist("admin") do |request|
       request.path == "/admin"
@@ -36,15 +38,15 @@ describe "Safelisting an IP subnet" do
   end
 
   it "notifies when the request is safe" do
-    notification_type = nil
-
     ActiveSupport::Notifications.subscribe("safelist.rack_attack") do |_name, _start, _finish, _id, payload|
-      notification_type = payload[:request].env["rack.attack.match_type"]
+      notifications.push(payload)
     end
 
     get "/admin", {}, "REMOTE_ADDR" => "5.6.0.0"
 
     assert_equal 200, last_response.status
-    assert_equal :safelist, notification_type
+    assert_equal 1, notifications.size
+    notification = notifications.pop
+    assert_equal :safelist, notification[:request].env["rack.attack.match_type"]
   end
 end

spec/acceptance/throttling_spec.rb

@@ -4,6 +4,8 @@ require_relative "../spec_helper"
 require "timecop"
 
 describe "#throttle" do
+  let(:notifications) { [] }
+
   before do
     Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new
   end
@@ -138,42 +140,31 @@ describe "#throttle" do
       request.ip
     end
 
-    notification_matched = nil
-    notification_type = nil
-    notification_data = nil
-    notification_discriminator = nil
-
     ActiveSupport::Notifications.subscribe("throttle.rack_attack") do |_name, _start, _finish, _id, payload|
-      notification_matched = payload[:request].env["rack.attack.matched"]
-      notification_type = payload[:request].env["rack.attack.match_type"]
-      notification_data = payload[:request].env['rack.attack.match_data']
-      notification_discriminator = payload[:request].env['rack.attack.match_discriminator']
+      notifications.push(payload)
     end
 
     get "/", {}, "REMOTE_ADDR" => "5.6.7.8"
 
     assert_equal 200, last_response.status
-    assert_nil notification_matched
-    assert_nil notification_type
-    assert_nil notification_data
-    assert_nil notification_discriminator
+    assert notifications.empty?
 
     get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
 
     assert_equal 200, last_response.status
-    assert_nil notification_matched
-    assert_nil notification_type
-    assert_nil notification_data
-    assert_nil notification_discriminator
+    assert notifications.empty?
 
     get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
 
     assert_equal 429, last_response.status
-    assert_equal "by ip", notification_matched
-    assert_equal :throttle, notification_type
-    assert_equal 60, notification_data[:period]
-    assert_equal 1, notification_data[:limit]
-    assert_equal 2, notification_data[:count]
-    assert_equal "1.2.3.4", notification_discriminator
+
+    assert_equal 1, notifications.size
+    notification = notifications.pop
+    assert_equal "by ip", notification[:request].env["rack.attack.matched"]
+    assert_equal :throttle, notification[:request].env["rack.attack.match_type"]
+    assert_equal 60, notification[:request].env["rack.attack.match_data"][:period]
+    assert_equal 1, notification[:request].env["rack.attack.match_data"][:limit]
+    assert_equal 2, notification[:request].env["rack.attack.match_data"][:count]
+    assert_equal "1.2.3.4", notification[:request].env["rack.attack.match_discriminator"]
   end
 end

spec/acceptance/track_spec.rb

@@ -3,27 +3,26 @@
 require_relative "../spec_helper"
 
 describe "#track" do
+  let(:notifications) { [] }
+
   it "notifies when track block returns true" do
     Rack::Attack.track("ip 1.2.3.4") do |request|
       request.ip == "1.2.3.4"
     end
 
-    notification_matched = nil
-    notification_type = nil
-
     ActiveSupport::Notifications.subscribe("track.rack_attack") do |_name, _start, _finish, _id, payload|
-      notification_matched = payload[:request].env["rack.attack.matched"]
-      notification_type = payload[:request].env["rack.attack.match_type"]
+      notifications.push(payload)
     end
 
     get "/", {}, "REMOTE_ADDR" => "5.6.7.8"
 
-    assert_nil notification_matched
-    assert_nil notification_type
+    assert notifications.empty?
 
     get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
 
-    assert_equal "ip 1.2.3.4", notification_matched
-    assert_equal :track, notification_type
+    assert_equal 1, notifications.size
+    notification = notifications.pop
+    assert_equal "ip 1.2.3.4", notification[:request].env["rack.attack.matched"]
+    assert_equal :track, notification[:request].env["rack.attack.match_type"]
   end
 end

spec/acceptance/track_throttle_spec.rb

@@ -4,6 +4,8 @@ require_relative "../spec_helper"
 require "timecop"
 
 describe "#track with throttle-ish options" do
+  let(:notifications) { [] }
+
   it "notifies when throttle goes over the limit without actually throttling requests" do
     Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new
 
@@ -11,43 +13,35 @@ describe "#track with throttle-ish options" do
       request.ip
     end
 
-    notification_matched = nil
-    notification_type = nil
-
     ActiveSupport::Notifications.subscribe("track.rack_attack") do |_name, _start, _finish, _id, payload|
-      notification_matched = payload[:request].env["rack.attack.matched"]
-      notification_type = payload[:request].env["rack.attack.match_type"]
+      notifications.push(payload)
     end
 
     get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
 
-    assert_nil notification_matched
-    assert_nil notification_type
+    assert notifications.empty?
 
     assert_equal 200, last_response.status
 
     get "/", {}, "REMOTE_ADDR" => "5.6.7.8"
 
-    assert_nil notification_matched
-    assert_nil notification_type
+    assert notifications.empty?
 
     assert_equal 200, last_response.status
 
     get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
 
-    assert_equal "by ip", notification_matched
-    assert_equal :track, notification_type
+    assert_equal 1, notifications.size
+    notification = notifications.pop
+    assert_equal "by ip", notification[:request].env["rack.attack.matched"]
+    assert_equal :track, notification[:request].env["rack.attack.match_type"]
 
     assert_equal 200, last_response.status
 
     Timecop.travel(60) do
-      notification_matched = nil
-      notification_type = nil
-
       get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
 
-      assert_nil notification_matched
-      assert_nil notification_type
+      assert notifications.empty?
 
       assert_equal 200, last_response.status
     end

spec/rack_attack_track_spec.rb

@@ -3,21 +3,7 @@
 require_relative 'spec_helper'
 
 describe 'Rack::Attack.track' do
-  let(:counter_class) do
-    Class.new do
-      def self.incr
-        @counter += 1
-      end
-
-      def self.reset
-        @counter = 0
-      end
-
-      def self.check
-        @counter
-      end
-    end
-  end
+  let(:notifications) { [] }
 
   before do
     Rack::Attack.track("everything") { |_req| true }
@@ -34,19 +20,18 @@ describe 'Rack::Attack.track' do
 
   describe "with a notification subscriber and two tracks" do
     before do
-      counter_class.reset
       # A second track
       Rack::Attack.track("homepage") { |req| req.path == "/" }
 
-      ActiveSupport::Notifications.subscribe("track.rack_attack") do |*_args|
-        counter_class.incr
+      ActiveSupport::Notifications.subscribe("track.rack_attack") do |_name, _start, _finish, _id, payload|
+        notifications.push(payload)
       end
 
       get "/"
     end
 
     it "should notify twice" do
-      _(counter_class.check).must_equal 2
+      _(notifications.size).must_equal 2
     end
   end