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

Add new RSpecRails/SettingsOverwritten cop #22

Closed
wants to merge 1 commit into from
Closed

Conversation

ydah
Copy link
Member

@ydah ydah commented Apr 9, 2024

One problem I encountered recently was that I was directly rewriting Settings in one test, resulting in flaky tests in which other tests affected by the rewritten Settings succeeded or failed depending on the order of execution, making it difficult to determine the cause. Therefore, in this PR, I would like to propose the addition of cop to encourage the use of mocks if there are any overwriting Settings.

# bad
Settings.foo = 'bar'
Settings[:foo] = 'bar'
Settings.foo[:bar] = 'baz'
Settings.foo += 'bar'
Settings.foo << 'bar'

# good
allow(Settings).to receive(:foo).and_return('bar')
allow(Settings).to receive(:[]).with(:foo).and_return('bar')
allow(Settings.foo).to receive(:[]).with(:bar).and_return('baz')
allow(Settings).to receive(:foo).and_return(Settings.foo + "bar")

Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

  • Added the new cop to config/default.yml.
  • The cop is configured as Enabled: pending in config/default.yml.
  • The cop documents examples of good and bad code.
  • The tests assert both that bad code is reported and that good code is not reported.
  • Set VersionAdded: "<<next>>" in default/config.yml.

@ydah ydah requested a review from a team as a code owner April 9, 2024 08:50
One problem I encountered recently was that I was directly rewriting `Settings` in one test, resulting in flaky tests in which other tests affected by the rewritten `Settings` succeeded or failed depending on the order of execution, making it difficult to determine the cause.
Therefore, in this PR, I would like to propose the addition of cop to encourage the use of mocks if there are any overwriting `Settings`.
@Darhazer
Copy link
Member

Darhazer commented Apr 9, 2024

This can happen with any kind of object that you change, and do not restore back. Perhaps a global rspec cop ObjectNotStubbed should be implemented.

@bquorning
Copy link
Contributor

I’m not sure which Settings class this is. As @Darhazer mentions, this problem applies to all globals / constants / class variables. But I don’t think we can reliably detect mutable operations. And even if we could, in most cases I would probably prefer to reset the object after testing, than to test against a stub or mock.

@ydah
Copy link
Member Author

ydah commented Apr 9, 2024

Uh. I had a big mistake. Settings are not appropriate since they are based on config gems.
https://github.com/rubyconfig/config

Also, I agree that it is hard to detect all mutable operations, so I will just close this PR.

@ydah ydah closed this Apr 9, 2024
@ydah ydah deleted the ydah/new_cop branch April 9, 2024 11:03
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

Successfully merging this pull request may close these issues.

3 participants