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 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
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
-
!17705 (merged) - Add RuboCop to unify have_gitlab_http_status(:symbol)
-
!24214 (merged) - Mention named HTTP status codes in dev docs -
!23220 (merged) - spec/support/shared_examples/**/*
-
!23295 (merged) - ee/spec/support/shared_examples/**/*
-
!23554 (merged) - {,ee/}spec/features/**/*
-
!23735 (merged) - {,ee/}spec/controllers/*.rb
-
!23736 (merged) - {,ee/}spec/controllers/requests/*.rb
-
!23801 (merged) - {,ee/}spec/controllers/projects/*.rb
-
!24209 (merged) - {,ee/}spec/controllers/projects/**/*.rb
-
!24211 (merged) - {,ee/}spec/controllers/groups/**/*.rb
-
!24212 (merged) - {,ee/}spec/support/**/*
+{,ee/}spec/controllers/**/*
-
!25162 (merged) - {,ee/}spec/support/*/**/*.rb
-
!25166 (merged) - {,ee/}spec/requests/{groups,projects,repositories}/**/*
-
!25168 (merged) - {,ee/}spec/requests/api/[a-f]*.rb
-
!25170 (merged) - {,ee/}spec/requests/api/[a-l]*.rb
-
!25173 (merged) - {,ee/}spec/requests/api/[a-o]*.rb
-
!25175 (merged) - {,ee/}spec/requests/api/[a-p]*.rb
-
!25177 (merged) - {,ee/}spec/requests/api/[a-s]*.rb
-
!25178 (merged) - {,ee/}/spec/**/*
🔚
Edited by Peter Leitzen