#670 causes double-notifying in tests

[jarkko]

Describe the bug

The ActiveJob support added in #670 doesn't recognise ActiveJob::QueueAdapters::TestAdapter as an existing adapter in EXISTING_INTEGRATIONS.

Thus, with the following job code:

class ProcessAutopilotNotificationJob < ApplicationJob
  queue_as :low
  class NoPhoneNumberError < StandardError; end
  notify = ->(job, error) { Bugsnag.notify(error) }
  discard_on NoPhoneNumberError, &notify
…

The following test now fails after updating to 6.22:

    it "raises if no phone number can be found" do
      expect(Bugsnag).to receive(:notify).with(an_instance_of(ProcessAutopilotNotificationJob::NoPhoneNumberError))
      ProcessAutopilotNotificationJob.perform_now(user,
        key: "sms_test_message",
        date: "2018-01-10",
        autopilot_data: autopilot_data)
    end

…because Bugsnag.notify is now called twice in the tests, once in the notify lambda above, and once by the new code in the gem. If ActiveJob::QueueAdapters::TestAdapter would be listed in EXISTING_INTEGRATIONS, this would not happen, but I'm not sure whether it would have some other unwanted consequences.

[imjoehaines]

This looks like it's working as expected — the error is notified once by the discard_on and will now also be notified by the new integration. You should be able to remove the Bugsnag.notify call from discard_on as it is no longer necessary, and the test should start passing again:

class ProcessAutopilotNotificationJob < ApplicationJob
  queue_as :low
  class NoPhoneNumberError < StandardError; end
-  notify = ->(job, error) { Bugsnag.notify(error) }
-  discard_on NoPhoneNumberError, &notify
+  discard_on NoPhoneNumberError
…

Adding the TestAdapter to EXISTING_INTEGRATIONS would mean Bugsnag wouldn't rescue from errors raised in jobs in tests. This would make tests like your example fail if there wasn't a second notify call, even though the error would be notified when using a non-test adapter

The EXISTING_INTEGRATIONS list is to prevent double reporting and/or duplicate metadata being recorded when using a queue library that we already have an integration with, e,g, Sidekiq, which doesn't apply to the test adapter

Hope that helps! I'm going to close this issue but let me know if I've misunderstood something and I'll reopen it 🙂

[jarkko]

Thanks, that makes sense!