Merge branch 'next' into cawllec/auto_at_exit

.github/PULL_REQUEST_TEMPLATE.md

@@ -0,0 +1,84 @@
+## Goal
+
+<!-- What is the intent of this change?
+e.g. "When initializing the Bugsnag client, it is currently difficult to (...)
+      this change simplifies the process by (...)"
+
+     "Improves the performance of data filtering"
+
+     "Adds additional test coverage to multi-threaded use of Configuration
+      objects"
+-->
+
+<!-- For new features, include design documentation:
+
+## Design
+
+Why was this approach to the goal used?
+
+-->
+
+## Changeset
+
+<!-- What structures or properties or functions were:
+
+### Added
+
+### Removed
+
+### Changed
+
+-->
+
+## Tests
+
+<!-- How was this change tested? What manual and automated tests were
+     run/added? -->
+
+## Discussion
+
+### Alternative Approaches
+
+<!-- What other approaches were considered or discussed? -->
+
+### Outstanding Questions
+
+<!-- Are there any parts of the design or the implementation which seem
+     less than ideal and that could require additional discussion?
+     List here: -->
+
+### Linked issues
+
+<!--
+
+Fixes #
+Related to #
+
+-->
+
+## Review
+
+<!-- When submitting for review, consider the points for self-review and the
+     criteria which will be used for secondary review -->
+
+For the submitter, initial self-review:
+
+- [ ] Commented on code changes inline explain the reasoning behind the approach
+- [ ] Reviewed the test cases added for completeness and possible points for discussion
+- [ ] A changelog entry was added for the goal of this pull request
+- [ ] Check the scope of the changeset - is everything in the diff required for the pull request?
+- This pull request is ready for:
+  - [ ] Initial review of the intended approach, not yet feature complete
+  - [ ] Structural review of the classes, functions, and properties modified
+  - [ ] Final review
+
+For the pull request reviewer(s), this changeset has been reviewed for:
+
+- [ ] Consistency across platforms for structures or concepts added or modified
+- [ ] Consistency between the changeset and the goal stated above
+- [ ] Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
+- [ ] Usage friction - is the proposed change in usage cumbersome or complicated?
+- [ ] Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
+- [ ] Concurrency concerns - if components are accessed asynchronously, what issues will arise
+- [ ] Thoroughness of added tests and any missing edge cases
+- [ ] Idiomatic use of the language

lib/bugsnag/integrations/delayed_job.rb

@@ -30,7 +30,8 @@ def error(job, error)
               overrides[:job][:queue] = queue
             end
             if job.respond_to?(:attempts)
-              overrides[:job][:attempts] = "#{job.attempts + 1} / #{Delayed::Worker.max_attempts}"
+              max_attempts = job.respond_to?(:max_attempts) ? job.max_attempts : Delayed::Worker.max_attempts
+              overrides[:job][:attempts] = "#{job.attempts + 1} / #{max_attempts}"
               # +1 as "attempts" is really previous attempts AFAICT, certainly it starts at 0.
             end
             if payload = job.payload_object

spec/integration_spec.rb

@@ -57,10 +57,10 @@
 
       Bugsnag.notify 'yo'
 
-      Process.fork do
+      pid = Process.fork do
         Bugsnag.notify 'yo too'
       end
-      Process.wait
+      Process.wait(pid)
 
       expect(queue.length).to eq(2)
     end

spec/integrations/delayed_job_spec.rb

@@ -4,12 +4,115 @@
 
 RSpec.describe ::Delayed::Plugins::Bugsnag do
   describe '#error' do
-    it 'should not raise exception' do
+    it 'should set metadata correctly with max_attempts' do
       payload = Object.new
       payload.extend(described_class::Notify)
+      job = double
+      allow(job).to receive_messages(
+        :id => "TEST",
+        :queue => "TEST_QUEUE",
+        :attempts => 0,
+        :max_attempts => 3,
+        :payload_object => {
+          :id => "PAYLOAD_ID",
+          :display_name => "PAYLOAD_DISPLAY_NAME",
+          :method_name => "PAYLOAD_METHOD_NAME",
+          :args => [
+            "SOME",
+            "TEST",
+            "ARGS"
+          ]
+        }
+      )
+
+      expect do
+        payload.error(job, '')
+      end.not_to raise_error
+
+      expect(Bugsnag).to have_sent_notification{ |payload, headers|
+        event = get_event_from_payload(payload)
+        expect(event["severity"]).to eq("error")
+        expect(event["severityReason"]).to eq({
+          "type" => "unhandledExceptionMiddleware",
+          "attributes" => {
+            "framework" => "DelayedJob"
+          }
+        })
+        expect(event["metaData"]["job"]).to eq({
+          "class" => job.class.name,
+          "id" => "TEST",
+          "queue" => "TEST_QUEUE",
+          "attempts" => "1 / 3",
+          "payload" => {
+            "class" => {}.class.name,
+            "args" => {
+              "id" => "PAYLOAD_ID",
+              "display_name" => "PAYLOAD_DISPLAY_NAME",
+              "method_name" => "PAYLOAD_METHOD_NAME",
+              "args" => [
+                "SOME",
+                "TEST",
+                "ARGS"
+              ]
+            }
+          }
+        })
+      }
+    end
+
+    it 'should set metadata correctly without max_attempts' do
+      payload = Object.new
+      payload.extend(described_class::Notify)
+      job = double
+      allow(job).to receive_messages(
+        :id => "TEST",
+        :queue => "TEST_QUEUE",
+        :attempts => 0,
+        :payload_object => {
+          :id => "PAYLOAD_ID",
+          :display_name => "PAYLOAD_DISPLAY_NAME",
+          :method_name => "PAYLOAD_METHOD_NAME",
+          :args => [
+            "SOME",
+            "TEST",
+            "ARGS"
+          ]
+        }
+      )
+
       expect do
-        payload.error(double('job', id: 1, payload_object: nil), '')
+        payload.error(job, '')
       end.not_to raise_error
+
+      expect(Bugsnag).to have_sent_notification{ |payload, headers|
+        event = get_event_from_payload(payload)
+        expect(event["severity"]).to eq("error")
+        expect(event["severityReason"]).to eq({
+          "type" => "unhandledExceptionMiddleware",
+          "attributes" => {
+            "framework" => "DelayedJob"
+          }
+        })
+        expect(event["metaData"]["job"]).to eq({
+          "class" => job.class.name,
+          "id" => "TEST",
+          "queue" => "TEST_QUEUE",
+          "attempts" => "1 / #{Delayed::Worker.max_attempts}",
+          "payload" => {
+            "class" => {}.class.name,
+            "args" => {
+              "id" => "PAYLOAD_ID",
+              "display_name" => "PAYLOAD_DISPLAY_NAME",
+              "method_name" => "PAYLOAD_METHOD_NAME",
+              "args" => [
+                "SOME",
+                "TEST",
+                "ARGS"
+              ]
+            }
+          }
+        })
+      }
     end
   end
 end

spec/integrations/rack_spec.rb

@@ -62,8 +62,8 @@
     before do
       unless defined?(::Rack)
         @mocked_rack = true
-        class ::Rack
-          class ::Request
+        class Rack
+          class Request
           end
         end
       end
@@ -94,7 +94,7 @@ class ::Request
         :referer => "referer",
         :fullpath => "/TEST_PATH"
       )
-      expect(::Rack::Request).to receive(:new).with(rack_env).and_return(rack_request)
+      expect(Rack::Request).to receive(:new).with(rack_env).and_return(rack_request)
 
       report = double("Bugsnag::Report")
       allow(report).to receive(:request_data).and_return({