Check for successful HTTP responses when testing integrations, rather than specific status codes

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

• Close this issue

Background

The following discussion from !67524 (merged) should be addressed:

• @nmilojevic1 started a discussion: (+2 comments)

  praise: for using cover? instead of include?

  Are we going to need this in other places as well? I see that we are checking != 200 in integrations/jenkins.rb and integrations/bamboo.rb

  nitpick: we could maybe extract range to the constant and this to a method, something like:

    SUCCESSFUL_STATUSES = (200..299).freeze
    ...
    def success?(status)
      (200..299).cover?(status)
    end
    
    ...
    success: success?(result[:http_status])

Details

The following integrations currently check for specific HTTP status codes in their #test method:

• app/models/integrations/prometheus.rb
• app/models/integrations/datadog.rb
• app/models/integrations/jenkins.rb
• app/models/integrations/packagist.rb

It's also worth noting that for integrations with webhooks, we show all 2xx responses as "successful" in the "Recent deliveries" section on the integration form (determined with WebHookLog#success?).

Proposal

Relax these tests to accept all 2xx responses, or maybe we could also use response.success? to avoid checking for specific status codes. Currently response is only available in WebHookService, so this would need some refactoring.

Or could we simplify these tests to work similarly to other integrations, where we just assert that we got a response? (see e.g. the default Integration#test)

Or do we maybe need to make the other integration tests stricter, to also check for a successful HTTP response?