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

Suggested cop: Prefer built-in time helpers to Timecop #20

Open
andyw8 opened this issue Nov 30, 2018 · 4 comments · May be fixed by rubocop/rubocop-rspec_rails#48
Open

Suggested cop: Prefer built-in time helpers to Timecop #20

andyw8 opened this issue Nov 30, 2018 · 4 comments · May be fixed by rubocop/rubocop-rspec_rails#48

Comments

@andyw8
Copy link
Contributor

andyw8 commented Nov 30, 2018

Most of Timecop's behaviour is now built into Rails.

https://api.rubyonrails.org/v5.1/classes/ActiveSupport/Testing/TimeHelpers.html

Related: rubocop/rspec-style-guide#71

@pirj
Copy link
Member

pirj commented Aug 7, 2024

This can be closed. The cop moved to another extension rubocop/rubocop-rspec_rails#48

@TobiasBales
Copy link

But not everybody uses RSpec and, afaik, rails new defaults to minitest.
So adding this to rubocop-rails over ruboyopc-rspec_rails seems highly desirable

@pirj
Copy link
Member

pirj commented Jan 27, 2025

Fair enough!

Actually, a cop targeting minitest would be be a better fit, as TimeHelpers are (to my best understanding) always included, and this means that it tears down automatically. Which is, unfortunately, not so simple with RSpec.

@sambostock
Copy link
Contributor

cop targeting minitest would be be a better fit

What do you mean by this?


Context

Enforcing TimeHelpers over Timecop is indeed straightforward when the test is using ActiveSupport::TestCase (or some subclass), as it includes TimeHelpers, relies on the after_teardown Minitest lifecycle hook (which makes sense, as TestCase inherits directly from Minitest::Test).

For RSpec specs using rails_helper, the Minitest compatibility layer is present, so after_teardown runs and using TimeHelpers is safe, but as you allude to, most of the uncertainty that held up #38 was dealing with the edge case of specs using spec_helper only, which does not load the compatibility layer (unless the host app does so), meaning TimeHelpers does not clean up after itself.

So yes, not having to think about RSpec dramatically simplifies things, but given that Minitest is the default (via ActiveSupport::TestCase), AFAIK there is no separate rubocop-minitest_rails equivalent to rubocop-rspec_rails.

The simplest thing would probably be for the cop to be disabled by default if the project is using RSpec, but expose a config to override that if the project maintainer chooses to do so (e.g. if they add the Minitest lifecycle hooks compatibility layer to their spec_helper).

Alternatively, rubocop-rails could add the cop, and rubocop-rspec_rails could disable it by default.


But unless something has changed, the conclusion in #38 (although I still disagree) was that a Rails/Timecop cop is not appropriate for rubocop-rails1, and I don't know that anyone wants to create/maintain a gem2 just for this one cop, so I don't see a way forward.

Footnotes

  1. I will gladly re-open the PR if this changes.

  2. Neither rubocop-timecop nor rubocop-rails-timecop seem appropriate, as they would have poor discoverability, and do not fit into the pattern of rubocop-timecop provides cops for users of Timecop, as its only cop would be one that says not to use Timecop.

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 a pull request may close this issue.

4 participants