[go: up one dir, main page]

Skip to content

RSpec shared example `412 response` naming is non-descriptive

https://gitlab.com/gitlab-org/gitlab/-/blob/9082b8ce7e94449b73acd7e93c64501a86e94a3d/spec/support/shared_examples/requests/api/status_shared_examples.rb#L70

This shared example for the REST API is named poorly. We should be describing what this covers better. I don't think many devs even understand what this covers looking at the actual logic.

https://gitlab.com/gitlab-org/gitlab/-/blob/9082b8ce7e94449b73acd7e93c64501a86e94a3d/lib/api/helpers.rb#L47-60

    def destroy_conditionally!(resource, last_updated: nil)
      last_updated ||= resource.updated_at

      check_unmodified_since!(last_updated)

      status 204
      body false

      if block_given?
        yield resource
      else
        resource.destroy
      end
    end

We have this API helper method to conditionally destroy a resource which ensure that the record hasn't been modified since we fetched it from the database (or a specified value in the kwargs)

I think this method naming isn't great either but that's another issue.

The shared examples should be named something like "it can be destroyed conditionally". Currently we have specs that duplicate this test because the naming is confusing e.g. https://gitlab.com/gitlab-org/gitlab/-/blob/9082b8ce7e94449b73acd7e93c64501a86e94a3d/spec/requests/api/protected_tags_spec.rb#L216-224

      it 'unprotects a single Protected Tag' do
        delete api("/projects/#{project.id}/protected_tags/#{tag_name}", user)

        expect(response).to have_gitlab_http_status(:no_content)
      end

      it_behaves_like '412 response' do
        let(:request) { api("/projects/#{project.id}/protected_tags/#{tag_name}", user) }
      end

If we refactor this to remove the it 'unprotects a single Protected Tag' spec it looks like we are testing a success flow.