diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 000000000..4bd482943 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,84 @@ +## Goal + + + + + +## Changeset + + + +## Tests + + + +## Discussion + +### Alternative Approaches + + + +### Outstanding Questions + + + +### Linked issues + + + +## 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 diff --git a/lib/bugsnag/integrations/delayed_job.rb b/lib/bugsnag/integrations/delayed_job.rb index 1b0abbe2b..50bf39e71 100644 --- a/lib/bugsnag/integrations/delayed_job.rb +++ b/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 diff --git a/spec/integration_spec.rb b/spec/integration_spec.rb index cb71b1e03..fcb3b3b24 100644 --- a/spec/integration_spec.rb +++ b/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 diff --git a/spec/integrations/delayed_job_spec.rb b/spec/integrations/delayed_job_spec.rb index 6813ed371..7c9b6435d 100644 --- a/spec/integrations/delayed_job_spec.rb +++ b/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 diff --git a/spec/integrations/rack_spec.rb b/spec/integrations/rack_spec.rb index 85717cbe5..37864bdb2 100644 --- a/spec/integrations/rack_spec.rb +++ b/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({