more tests

newrelic · Jun 24, 2021 · e93daff · e93daff
1 parent 16b5307
commit e93daff
Show file tree

Hide file tree

Showing 4 changed files with 139 additions and 44 deletions.
diff --git a/lib/new_relic/agent/error_filter.rb b/lib/new_relic/agent/error_filter.rb
@@ -10,6 +10,10 @@ module Agent
     class ErrorFilter
 
       def initialize
+        reset
+      end
+
+      def reset
         @ignore_errors, @ignore_classes, @expected_classes = [], [], []
         @ignore_messages, @expected_messages = {}, {}
         @ignore_status_codes, @expected_status_codes = [], []
@@ -63,42 +67,48 @@ def fetch_agent_config(cfg)
         NewRelic::Agent.config[:"error_collector.#{cfg}"]
       end
 
-      def ignore(errors)
-        case errors
-        when Array
-          @ignore_classes += errors
-          log_filter(:ignore_classes, errors)
-        when Hash
-          @ignore_messages.update(errors)
-          log_filter(:ignore_messages, errors)
-        when String
-          if errors.match(/^[\d\,\-]+$/)
-            @ignore_status_codes += parse_status_codes(errors)
-            log_filter(:ignore_status_codes, errors)
-          else
-            new_ignore_classes = errors.split(',').map!(&:strip)
-            @ignore_classes += new_ignore_classes
-            log_filter(:ignore_classes, new_ignore_classes)
+      # A generic method for adding ignore filters manually. This is kept for compatibility
+      # with the previous ErrorCollector#ignore method, and adds some flexibility for adding
+      # different ignore/expected error types by examining each argument.
+      def ignore(*args)
+        args.each do |errors|
+          case errors
+          when Array
+            errors.each { |e| ignore(e) }
+          when Hash
+            @ignore_messages.update(errors)
+            log_filter(:ignore_messages, errors)
+          when String
+            if errors.match(/^[\d\,\-]+$/)
+              @ignore_status_codes |= parse_status_codes(errors)
+              log_filter(:ignore_status_codes, errors)
+            else
+              new_ignore_classes = errors.split(',').map!(&:strip)
+              @ignore_classes |= new_ignore_classes
+              log_filter(:ignore_classes, new_ignore_classes)
+            end
           end
         end
       end
 
-      def expect(errors)
-        case errors
-        when Array
-          @expected_classes += errors
-          log_filter(:expected_classes, errors)
-        when Hash
-          @expected_messages.update(errors)
-          log_filter(:expected_messages, errors)
-        when String
-          if errors.match(/^[\d\,\-]+$/)
-            @expected_status_codes += parse_status_codes(errors)
-            log_filter(:expected_status_codes, errors)
-          else
-            new_expected_classes = errors.split(',').map!(&:strip)
-            @expected_classes += new_expected_classes
-            log_filter(:expected_classes, new_expected_classes)
+      # See #ignore above.
+      def expect(*args)
+        args.each do |errors|
+          case errors
+          when Array
+            errors.each { |e| expect(e) }
+          when Hash
+            @expected_messages.update(errors)
+            log_filter(:expected_messages, errors)
+          when String
+            if errors.match(/^[\d\,\-]+$/)
+              @expected_status_codes |= parse_status_codes(errors)
+              log_filter(:expected_status_codes, errors)
+            else
+              new_expected_classes = errors.split(',').map!(&:strip)
+              @expected_classes |= new_expected_classes
+              log_filter(:expected_classes, new_expected_classes)
+            end
           end
         end
       end
@@ -137,7 +147,7 @@ def parse_status_codes(codes)
           m = code.match(/(\d{3})(-\d{3})?/)
           if m.nil? || m[1].nil?
             ::NewRelic::Agent.logger.warn("Invalid HTTP status code: '#{code}'; ignoring config")
-            return []
+            next
           end
           if m[2]
             result += (m[1]..m[2].tr('-', '')).to_a.map(&:to_i)

diff --git a/test/multiverse/suites/rails/error_tracing_test.rb b/test/multiverse/suites/rails/error_tracing_test.rb
@@ -36,6 +36,10 @@ def ignored_error
     raise NewRelic::TestHelpers::Exceptions::IgnoredError.new('this error should not be noticed')
   end
 
+  def ignored_status_code
+    render body: "Protip: you probably shouldn't ignore 500 errors", status: 500
+  end
+
   def server_ignored_error
     raise NewRelic::TestHelpers::Exceptions::ServerIgnoredError.new('this is a server ignored error')
   end
@@ -231,6 +235,14 @@ def test_should_not_notice_filtered_errors
            'Noticed an error that should have been ignored')
   end
 
+  def test_should_not_notice_ignored_status_codes
+    with_config(:'error_collector.ignore_status_codes' => '500') do
+      get '/error/ignored_status_code'
+
+      assert(errors.empty?, 'Noticed an error that should have been ignored')
+    end
+  end
+
   def test_should_notice_server_ignored_error_if_no_server_side_config
     get '/error/server_ignored_error'
     assert_error_reported_once('this is a server ignored error')
@@ -288,6 +300,7 @@ def test_should_not_increment_metrics_on_expected_error_errors
     ])
 
     assert_metrics_recorded("Apdex" => { :apdex_s => 1 })
+    assert_metrics_recorded(["ErrorsExpected/all"])
   end
 
 

diff --git a/test/new_relic/agent/error_collector_test.rb b/test/new_relic/agent/error_collector_test.rb
@@ -350,6 +350,17 @@ def test_ignore_error
         refute @error_collector.ignore?(error)
       end
 
+      def test_ignored_and_expected_error_is_ignored
+        error = AnError.new
+        with_config(:'error_collector.ignore_classes' => ['AnError'],
+                    :'error_collector.expected_classes' => ['AnError']) do
+          @error_collector.notice_error(AnError.new)
+
+          events = harvest_error_events
+          assert_equal 0, events.length
+        end
+      end
+
       def test_ignore_status_codes
         error = AnError.new
         with_config(:'error_collector.ignore_status_codes' => '400-408') do

diff --git a/test/new_relic/agent/error_filter_test.rb b/test/new_relic/agent/error_filter_test.rb
@@ -6,6 +6,7 @@
 
 class TestExceptionA < StandardError; end
 class TestExceptionB < StandardError; end
+class TestExceptionC < StandardError; end
 
 module NewRelic::Agent
   class ErrorFilter
@@ -34,19 +35,58 @@ def test_ignore_messages
       end
 
       def test_ignore_status_codes
-        with_config :'error_collector.ignore_status_codes' => '401,405-412,500' do
+        with_config :'error_collector.ignore_status_codes' => '401,405-409,501' do
           @error_filter.load_all
           assert @error_filter.ignore?(TestExceptionA.new, 401)
-          assert @error_filter.ignore?(TestExceptionA.new, 409)
+          assert @error_filter.ignore?(TestExceptionA.new, 501)
+          (405..409).each do |c|
+            assert @error_filter.ignore?(TestExceptionA.new, c)
+          end
           refute @error_filter.ignore?(TestExceptionA.new, 404)
         end
       end
 
+      def test_ignore_status_codes_by_array
+        with_config :'error_collector.ignore_status_codes' => ['401', '405-409', '501'] do
+          @error_filter.load_all
+          assert @error_filter.ignore?(TestExceptionA.new, 401)
+          assert @error_filter.ignore?(TestExceptionA.new, 501)
+          (405..409).each do |c|
+            assert @error_filter.ignore?(TestExceptionA.new, c)
+          end
+          refute @error_filter.ignore?(TestExceptionA.new, 404)
+        end
+      end
+
+      def test_ignore_method
+        @error_filter.ignore('TestExceptionA', ['ArgumentError'])
+        assert @error_filter.ignore?(TestExceptionA.new)
+        assert @error_filter.ignore?(ArgumentError.new)
+        refute @error_filter.ignore?(TestExceptionB.new)
+
+        @error_filter.ignore('TestExceptionB', {'TestExceptionC' => ['message one', 'message two']})
+        assert @error_filter.ignore?(TestExceptionB.new)
+        assert @error_filter.ignore?(TestExceptionC.new('message one'))
+        assert @error_filter.ignore?(TestExceptionC.new('message two'))
+        refute @error_filter.ignore?(TestExceptionC.new('message three'))
+
+        @error_filter.reset
+        refute @error_filter.ignore?(TestExceptionA.new)
+
+        @error_filter.ignore('401,405-409', ['500', '505-509'])
+        assert @error_filter.ignore?(TestExceptionA.new, 401)
+        assert @error_filter.ignore?(TestExceptionA.new, 407)
+        assert @error_filter.ignore?(TestExceptionA.new, 500)
+        assert @error_filter.ignore?(TestExceptionA.new, 507)
+        refute @error_filter.ignore?(TestExceptionA.new, 404)
+      end
+
       def test_skip_invalid_status_codes
         with_config :'error_collector.ignore_status_codes' => '401,sausage,foo-bar,500' do
           @error_filter.load_all
           refute @error_filter.ignore?(TestExceptionA.new, 400)
-          refute @error_filter.ignore?(TestExceptionA.new, 401)
+          assert @error_filter.ignore?(TestExceptionA.new, 401)
+          assert @error_filter.ignore?(TestExceptionA.new, 500)
         end
       end
 
@@ -59,12 +99,6 @@ def test_ignore_errors
         end
       end
 
-      def test_ignore_from_string
-        @error_filter.ignore('TestExceptionA,TestExceptionC')
-        assert @error_filter.ignore?(TestExceptionA.new)
-        refute @error_filter.ignore?(TestExceptionB.new)
-      end
-
       def test_expected_classes
         with_config :'error_collector.expected_classes' => ['TestExceptionA', 'TestExceptionC'] do
           @error_filter.load_all
@@ -84,13 +118,40 @@ def test_expected_messages
       end
 
       def test_expected_status_codes
-        with_config :'error_collector.expected_status_codes' => '401,405-412,500' do
+        with_config :'error_collector.expected_status_codes' => '401,405-409,501' do
           @error_filter.load_all
           assert @error_filter.expected?(TestExceptionA.new, 401)
-          assert @error_filter.expected?(TestExceptionA.new, 409)
+          assert @error_filter.expected?(TestExceptionA.new, 501)
+          (405..409).each do |c|
+            assert @error_filter.expected?(TestExceptionA.new, c)
+          end
           refute @error_filter.expected?(TestExceptionA.new, 404)
         end
       end
+
+      def test_expect_method
+        @error_filter.expect('TestExceptionA', ['ArgumentError'])
+        assert @error_filter.expected?(TestExceptionA.new)
+        assert @error_filter.expected?(ArgumentError.new)
+        refute @error_filter.expected?(TestExceptionB.new)
+
+        @error_filter.expect('TestExceptionB', {'TestExceptionC' => ['message one', 'message two']})
+        assert @error_filter.expected?(TestExceptionB.new)
+        assert @error_filter.expected?(TestExceptionC.new('message one'))
+        assert @error_filter.expected?(TestExceptionC.new('message two'))
+        refute @error_filter.expected?(TestExceptionC.new('message three'))
+
+        @error_filter.reset
+        refute @error_filter.expected?(TestExceptionA.new)
+
+        @error_filter.expect('401,405-409', ['500', '505-509'])
+        assert @error_filter.expected?(TestExceptionA.new, 401)
+        assert @error_filter.expected?(TestExceptionA.new, 407)
+        assert @error_filter.expected?(TestExceptionA.new, 500)
+        assert @error_filter.expected?(TestExceptionA.new, 507)
+        refute @error_filter.expected?(TestExceptionA.new, 404)
+      end
+
     end
   end
 end