Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RSpec/ScatteredSetup: false positive #1165

Closed
ololobus opened this issue Jul 9, 2021 · 17 comments
Closed

RSpec/ScatteredSetup: false positive #1165

ololobus opened this issue Jul 9, 2021 · 17 comments

Comments

@ololobus
Copy link

ololobus commented Jul 9, 2021

I am using rails 6 + rspec + rswag and have a following repeating pattern in my request specs:

  path '/clusters/{id}/delete' do
    post 'Delete cluster' do
      tags 'Cluster'
      operationId 'deleteCluster'
      produces 'application/json'
      parameter name: :id, in: :path, type: :integer

      response '200', 'success' do
        before do
          cleanup_operations(existing_cluster)
        end

        schema '$ref' => '#/components/schemas/OperationResult'
        run_test!
      end

      response '409', 'conflict' do
        schema '$ref' => '#/components/schemas/OperationResult'
        run_test!
      end
    end

The idea is that model does not allow concurrent operations, so if there are running ones it will return 409 code. But I also want to test 200 response, so I use a cleanup helper, which forcedly marks all operations as finished. This would be fine, but I have several similar endpoints to test, e.g. start, stop, etc. And for some responses I need before callback and for some of them I do not, thus I cannot declare before on the describe level.

However, RSpec/ScatteredSetup says:

spec/requests/api_controller/clusters_spec.rb:147:9: C: RSpec/ScatteredSetup: Do not define multiple before hooks in the same example group (also defined on lines 182, 215, 377, 444, 485, 546, 604, 635).
        before do ...
        ^^^^^^^^^
spec/requests/api_controller/clusters_spec.rb:182:9: C: RSpec/ScatteredSetup: Do not define multiple before hooks in the same example group (also defined on lines 147, 215, 377, 444, 485, 546, 604, 635).
        before do ...
        ^^^^^^^^^
spec/requests/api_controller/clusters_spec.rb:215:9: C: RSpec/ScatteredSetup: Do not define multiple before hooks in the same example group (also defined on lines 147, 182, 377, 444, 485, 546, 604, 635).
        before do ...
        ^^^^^^^^^
spec/requests/api_controller/clusters_spec.rb:377:9: C: RSpec/ScatteredSetup: Do not define multiple before hooks in the same example group (also defined on lines 147, 182, 215, 444, 485, 546, 604, 635).
        before do ...

and so on.

And I cannot obey, because it will ruin my test logic. Am I missing something?

@pirj
Copy link
Member

pirj commented Jul 9, 2021

Your example only has a single before, so it's hard for me to make a picture of the whole structure.

I could suggest configuring example groups to include response. See

and https://docs.rubocop.org/rubocop-rspec/usage.html#rspec-dsl-configuration for an idea.
Be extra careful with that, though, as the default settings might be overridden due to a bug.

@ololobus
Copy link
Author

ololobus commented Jul 9, 2021

The whole structure is more like:

describe 'Cluster' do
  path '/clusters/{id}/start' do
    post 'Start cluster' do
      tags 'Cluster'
      operationId 'startCluster'
      produces 'application/json'
      parameter name: :id, in: :path, type: :integer

      response '200', 'success' do
        before do
          cleanup_operations(existing_cluster)
        end

        schema '$ref' => '#/components/schemas/OperationResult'
        run_test!
      end

      response '409', 'conflict' do
        schema '$ref' => '#/components/schemas/OperationResult'
        run_test!
      end
    end

  path '/clusters/{id}/delete' do
    post 'Delete cluster' do
      tags 'Cluster'
      operationId 'deleteCluster'
      produces 'application/json'
      parameter name: :id, in: :path, type: :integer

      response '200', 'success' do
        before do
          cleanup_operations(existing_cluster)
        end

        schema '$ref' => '#/components/schemas/OperationResult'
        run_test!
      end

      response '409', 'conflict' do
        schema '$ref' => '#/components/schemas/OperationResult'
        run_test!
      end
    end
end

but with several more endpoints.

@pirj
Copy link
Member

pirj commented Jul 9, 2021

Can you please try configuring the DSL?
I would suggest copy and pasting the existing one, and adding response to ExampleGroups/Regular.
To test that other cops' config is not broken, add some offence and make sure it's still detected after your change.

@pirj
Copy link
Member

pirj commented Jul 9, 2021

In general, we could suggest rspec-swagger to follow this document https://docs.rubocop.org/rubocop-rspec/third_party_rspec_syntax_extensions.html, but the bug prevents from using this without breaking other cops.

@ololobus
Copy link
Author

ololobus commented Jul 9, 2021

Thanks for the response.

I could suggest configuring example groups to include response.
I would suggest copy and pasting the existing one, and adding response to ExampleGroups/Regular.

If I put response into the ExampleGroups -> Regular it complains about empty example groups:

spec/requests/api_controller/clusters_spec.rb:607:7: C: RSpec/EmptyExampleGroup: Empty example group detected.
      response '200', 'success' do
      ^^^^^^^^^^^^^^^^^^^^^^^^^
spec/requests/api_controller/clusters_spec.rb:638:7: C: RSpec/EmptyExampleGroup: Empty example group detected.
      response '200', 'success' do

@ololobus
Copy link
Author

ololobus commented Jul 9, 2021

Gotcha, response is not an example group logically, it is example itself, so what I need is something like that:

RSpec:
  Language:
    ExampleGroups:
      Regular:
        - path
        - post
        - put
        - get
        - delete
    Examples:
      Regular:
        - response

Thanks for pointing me in that direction. I got a bunch of other offences, probably since Rubocop/RSpec started properly analyzing my rspec file, but the initial problem is gone.

@ololobus
Copy link
Author

ololobus commented Jul 9, 2021

@pirj, so if I am right, what I can do to prevent someone else from hitting the same issue in the future?

Create issue / PR in the https://github.com/rswag/rswag asking to add config/rubocop-rspec.yml file with:

RSpec:
  Language:
    ExampleGroups:
      Regular:
        - path
        - post
        - put
        - get
        - delete
    Examples:
      Regular:
        - response

and follow https://docs.rubocop.org/rubocop-rspec/third_party_rspec_syntax_extensions.html#packaging-configuration-for-rubocop-rspec + https://github.com/palkan/action_policy/pull/138/files ?

@pirj
Copy link
Member

pirj commented Jul 9, 2021

response is effectively an example group if it allows for before in it, and it's an RSpec-provided hook.

palkan/action_policy#138 is more representative, where success is an example, but has no body https://github.com/palkan/action_policy/pull/138/files#diff-7a633d5704411354d8906e8b463afd2c72bb4f1f12c7049b0e63e410047db58eR10.

Same here, run_test! is the example https://github.com/rswag/rswag/blob/05e22c3bd7fff53d40cd8104678dab73d0b7aff7/rswag-specs/lib/rswag/specs/example_group_helpers.rb#L88
It can be treated as it_behaves_like (Includes/Examples in the configuration).

Create issue / PR in the https://github.com/rswag/rswag

Right.
I'd suggest mentioning the before mentioned bug, as this may ruin other cops.

@pirj
Copy link
Member

pirj commented Jul 21, 2021

@ololobus Any update?

@ololobus
Copy link
Author

Thanks for your previous comment, now I use:

RSpec:
  Language:
    ExampleGroups:
      Regular:
        - path
        - post
        - put
        - get
        - delete
        - response
    Examples:
      Regular:
        - run_test!

so far so good. I only receive reasonable warnings.

I have not prepared a PR yet, but I found a closed issue rswag/rswag#138. And it seems that they are not really willing to accept something like that. Although I want to try anyway

@pirj
Copy link
Member

pirj commented Jul 28, 2021

I went ahead and going to fix the bug I've mentioned earlier in rubocop/rubocop#9952

If rswag doesn't accept your patch, I suggest you creating a rubocop-rspec-swag gem with just the config/default.yml in it, to act as a glue between rubocop-rspec cops and rswag code.

Do you think there's anything actionable left, or we can close this ticket, @ololobus ?

@ololobus
Copy link
Author

@pirj, yeah, I think so. Thanks for responses, that helped me a lot. I'm closing this one

@Krupskis
Copy link

@ololobus did you start anything with the rubocop-rspec-swag gem that @pirj suggested?

@ololobus
Copy link
Author

@Krupskis, not yet

@Krupskis
Copy link

@ololobus Do you plan to do so? I would like to contribute

@ololobus
Copy link
Author

Yeah, I'm going to do it, but if you are willing to do it right now, then go ahead, sure 👍

@Krupskis
Copy link

@ololobus I would like to do it together, as I am a beginner, especially at gems, so having some guidance would be pretty cool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants