Add cop rule to enforce `have_gitlab_http_status(:symbol)`

In !19929 (merged) we've started enforcing symbolic HTTP status codes in controllers 🎉

Description of the proposal

This proposal enforces the usage of have_gitlab_http_status (already mentioned in Testing best practices) and the usage of symbolic HTTP status codes in specs by introducing a 👮 RuboCop rule RSpec/HaveGitlabHttpStatus.

Bad

expect(response).to have_http_status(200)
expect(response).to have_http_status(:ok)
expect(response).to have_gitlab_http_status(200)

Good

expect(response).to have_gitlab_http_status(:ok)

How does it look like in the code?

!17705 (merged) demonstrates the upcoming changes. Once this issue has been approved, I'll split that MR into smaller chunks to avoid conflicts and to allow faster reviews.

Before

Right now, we are mixing have_{,gitlab_}http_status({:symbol, numeric}) a lot which seems not very consistent:

Before
$ rg --sort path -o "have_(gitlab_)?http_status.(\d+|:\w+)." --no-filename  | sort | uniq -c | sort -g
      1 have_gitlab_http_status 204
      1 have_gitlab_http_status(300)
      1 have_gitlab_http_status 404
      1 have_gitlab_http_status(505)
      1 have_gitlab_http_status(:conflict)
      1 have_gitlab_http_status :forbidden
      1 have_gitlab_http_status(:moved_permanently)
      1 have_gitlab_http_status(:not_implemented)
      1 have_gitlab_http_status :ok
      1 have_http_status 201
      1 have_http_status 204
      1 have_http_status 401
      1 have_http_status(429)
      1 have_http_status :forbidden
      1 have_http_status :no_content
      1 have_http_status :unauthorized
      2 have_gitlab_http_status 200
      2 have_gitlab_http_status(303)
      2 have_gitlab_http_status :bad_request
      2 have_gitlab_http_status(:error)
      2 have_gitlab_http_status(:internal_server_error)
      2 have_gitlab_http_status(:service_unavailable)
      2 have_http_status(:unprocessable_entity)
      3 have_gitlab_http_status 200
      3 have_gitlab_http_status(412)
      3 have_gitlab_http_status(413)
      3 have_gitlab_http_status(503)
      3 have_gitlab_http_status :created
      3 have_http_status(400)
      4 have_gitlab_http_status 302
      4 have_gitlab_http_status(405)
      4 have_gitlab_http_status(406)
      4 have_gitlab_http_status(429)
      4 have_http_status(204)
      4 have_http_status(403)
      5 have_gitlab_http_status 400
      5 have_gitlab_http_status 403
      5 have_http_status(:created)
      6 have_http_status(:found)
      8 have_gitlab_http_status(:accepted)
      8 have_gitlab_http_status :not_found
      8 have_gitlab_http_status(:redirect)
      8 have_http_status(201)
      9 have_http_status(401)
     10 have_http_status(:bad_request)
     10 have_http_status(:no_content)
     11 have_gitlab_http_status(301)
     11 have_http_status(302)
     13 have_gitlab_http_status(:found)
     13 have_http_status(:ok)
     14 have_gitlab_http_status(304)
     14 have_http_status(404)
     15 have_gitlab_http_status(:created)
     15 have_gitlab_http_status(:unprocessable_entity)
     15 have_http_status(200)
     16 have_gitlab_http_status(500)
     16 have_http_status 200
     17 have_gitlab_http_status 201
     17 have_gitlab_http_status(202)
     18 have_http_status 429
     25 have_http_status(:not_found)
     27 have_gitlab_http_status(409)
     27 have_gitlab_http_status(:no_content)
     47 have_gitlab_http_status(:unauthorized)
     53 have_gitlab_http_status(:bad_request)
     57 have_gitlab_http_status(422)
     57 have_gitlab_http_status(:forbidden)
     73 have_gitlab_http_status(:success)
    110 have_gitlab_http_status(204)
    128 have_gitlab_http_status(:not_found)
    147 have_gitlab_http_status(302)
    161 have_gitlab_http_status(401)
    305 have_gitlab_http_status(:ok)
    336 have_gitlab_http_status(400)
    362 have_gitlab_http_status(201)
    376 have_gitlab_http_status(403)
    812 have_gitlab_http_status(404)
   1505 have_gitlab_http_status(200)

After applying the 👮 RuboCop rule the code base looks much more consistent:

After
$ rg --sort path -o "have_(gitlab_)?http_status.(\d+|:\w+)." --no-filename  | sort | uniq -c | sort -g
      *1 have_gitlab_http_status(200)
      *1 have_gitlab_http_status(200,
      *1 have_gitlab_http_status(201)
      *1 have_gitlab_http_status(500)
      1 have_gitlab_http_status(:http_version_not_supported)
      1 have_gitlab_http_status(:invalid)
      1 have_gitlab_http_status(:multiple_choices)
      1 have_gitlab_http_status(:not_implemented)
      1 have_gitlab_http_status :ok
      *1 have_http_status(200)
      *1 have_http_status(200,
      *1 have_http_status(204)
      *1 have_http_status(:invalid)
      *1 have_http_status(:success) 
      2 have_gitlab_http_status(:error)
      2 have_gitlab_http_status :forbidden
      2 have_gitlab_http_status(:see_other)
      *2 have_http_status(:ok)
      3 have_gitlab_http_status :bad_request
      3 have_gitlab_http_status(:payload_too_large)
      3 have_gitlab_http_status(:precondition_failed)
      4 have_gitlab_http_status :created
      4 have_gitlab_http_status(:method_not_allowed)
      4 have_gitlab_http_status(:not_acceptable)
      5 have_gitlab_http_status(:service_unavailable)
      8 have_gitlab_http_status(:redirect)
     10 have_gitlab_http_status :not_found
     12 have_gitlab_http_status(:moved_permanently)
     14 have_gitlab_http_status(:not_modified)
     17 have_gitlab_http_status(:internal_server_error)
     23 have_gitlab_http_status(:too_many_requests)
     25 have_gitlab_http_status(:accepted)
     28 have_gitlab_http_status(:conflict)
     74 have_gitlab_http_status(:success)
     74 have_gitlab_http_status(:unprocessable_entity)
    155 have_gitlab_http_status(:no_content)
    181 have_gitlab_http_status(:found)
    220 have_gitlab_http_status(:unauthorized)
    408 have_gitlab_http_status(:bad_request)
    409 have_gitlab_http_status(:created)
    443 have_gitlab_http_status(:forbidden)
    982 have_gitlab_http_status(:not_found)
   1868 have_gitlab_http_status(:ok)

* These usages stem from the cop and the specs

Tasks

  • Mention the proposal in the next backend weekly call and the #backend channel to encourage contribution
  • Proceed with the proposal once 50% of the maintainers have weighed in, and 80% of their votes are 👍
  • Once approved, mention it again in the next backend weekly call and the #backend channel

Used scripts

Show pending fixes per spec directory:

rg "have(_gitlab)?_http.*\b\d\d\d\b" spec ee/spec -c | sort -t : -n +1 | ruby -ne 'BEGIN { x = Hash.new(0) }; f, c = $_.split(":"); d = File.dirname(f); x[d] += c.to_i ; END { x.each { |f, c| puts "#{f}:#{c}" }}' | sort -t : +1 -n

MRs

/cc @gitlab-org/maintainers/rails-backend

Edited by Peter Leitzen