-
-
Notifications
You must be signed in to change notification settings - Fork 530
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
Removed timecop
gem in favor of travel_to
#351
Conversation
Oh that's cool. This makes sense to me but have we done it on any apps yet? |
@croaky Also, I'm not sure how we've gone about communicating dependency removals in the past, but we'd probably want to point users to the docs for I'd expect users of |
Timecop does other things as well. If we need |
@calebthompson |
Looks like, yeah. The only things in the README other than that are Works for me, but I haven't used it either. As far as documenting the change, NEWS.md makes sense. |
NEWS.md
Outdated
@@ -1,3 +1,12 @@ | |||
1.12.0 (May 23, 2014) | |||
|
|||
* Included [formulaic](https://github.com/thoughtbot/formulaic) for integration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@calebthompson @croaky I've never written release notes before. Is this good enough?
Changes like this require a semver bump, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect.
@croaky good to merge and bump the release? Should we ask for more opinions? |
@seanpdoyle I'd like to hold off on merging until someone reports using this successfully on one of or more of our apps. |
Apparently the two behave differently.
I'm not sure of how to reproduce the |
@seanpdoyle @arunagw @sgrif @sikachu @jferris How are we feeling about this PR? Can Timecop definitely be replaced by |
Just ran into a scenario where travel_to wouldn't work: Google's oauth provides an access token which expires. As part of the refresh process, google sends an amount of time for which the new token will be valid in the form of a number of seconds. To verify that the expiration time is properly set to now + number_of_seconds, I need to freeze time as well as travel to a known base time. |
It seems like for now, we should probably keep Timecop in Suspenders. I think now might be a good time to close the PR. If others feel differently, please feel free to re-open. |
Is the concern whether |
The gems we have in Suspenders should be gems we think we need on the majority of new apps. Clearance is our default choice for authentication but is not in Suspenders. If we don't think we need time freezing by default in projects, lets remove Timecop. I thought we did feel we need it, though. What do you do instead, @sgrif, if you're avoiding both Timecop and |
Dependency injection. :) |
How have we been testing OAuth lately? Have we been using timecop in these tests, or some other form of stubbing? It might be worth revisiting this now. |
@mike-burns this may indeed be a good time to revisit this. In fact |
It seems most issues reported here have been solved by the Rails team.
|
ea3f125
to
cf27bf6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot's changed in 9 years :) Go for it!
cf27bf6
to
229424e
Compare
It is time. |
With the release of `rails` 4.1, `Timecop.freeze` can be replaced with `travel_to` ```ruby Timecop.freeze 1.week.ago do # time sensitive code end travel_to 1.week.ago do # time sensitive code end ``` Read more in the [TimeHelpers](https://github.com/rails/rails/blob/a6f55fe257512731d7f3f41976648d99e9ec95be/activesupport/lib/active_support/testing/time_helpers.rb#L43) module.
229424e
to
830b9ec
Compare
Noting we still have a reference to Timecop here: https://github.com/thoughtbot/suspenders/blob/main/templates/spec_helper.rb However, this will be removed in https://github.com/thoughtbot/suspenders/tree/suspenders-3-0-0-testing-generator |
Thank you for catching that @stevepolitodesign! Are you interested in opening a PR to remove that line in the short term? If not, I'm happy to open one. |
Follow-up to #351 Now that `Timecop` has been removed, we no longer need to configure RSpec to use it.
@seanpdoyle I've opened #1155 to address this. |
With the release of
rails
4.1,Timecop.freeze
can be replaced withtravel_to
Read more in the TimeHelpers module
inspired by http://brandonhilkert.com/blog/rails-4-1-travel-to-test-helper