travel/travel_to travel time helpers, now raise on nested calls
#24890
Conversation
activesupport/CHANGELOG.md
Outdated
There was a problem hiding this comment.
Won't travel_back be what users want to call 90% of the times if they add a second travel_to in tests? Then shouldn't we just call it automatically for them? We could add an reset: false opt-out to travel and travel_to?
There was a problem hiding this comment.
Thats something I needed to discuss with @rafaelfranca . We had not discussed this scenario.
But essentially, we would like to achieve orderly calls. travel_to 2.days.from_now, travel_to 5.days.from_now, so makes sense.
There was a problem hiding this comment.
I prefer to also use the block version here but without the nesting. Could you change it?
vipulnsward
force-pushed
the
travel-to-raise
branch
2 times, most recently
from
da79bfb to
c7a1a6e
Compare
There was a problem hiding this comment.
Something like this?
There was a problem hiding this comment.
Do we need to do the initial subsequent dance a second time? Could do with some newlines :)
vipulnsward
force-pushed
the
travel-to-raise
branch
from
c7a1a6e to
b3daa50
Compare
|
@vipulnsward hey, you still interested in making this happen? 😁 |
|
Oh yes. Its still on my radar. |
vipulnsward
force-pushed
the
travel-to-raise
branch
2 times, most recently
from
6860c0f to
4eed2a7
Compare
|
Alright, ready for another round. |
There was a problem hiding this comment.
Does this correctly reset the time outside of block? In other words, the inner travel_to pushes to the stack but do we pop all the way?
Shouldn't we test that here?
There was a problem hiding this comment.
On the other hand I also find it strange that we support without a block travel within a block. Wasn't the point of this to prevent inner block calls?
There was a problem hiding this comment.
No, not unless you do an explicit travel_back, but that too will reset the complete state, we don't allow travel_back to previous state.
I think this is ok, since we explicity specify our preferred way is to not use multi-block or calls within a block.
There was a problem hiding this comment.
I think you can pass a Regex as the second argument to assert_raises which could simplify this check. Perhaps checking that part of the message is emitted is sufficient coverage rather than strictly asserting on the whole format 😁
|
How do we fare with putting |
|
This also breaks Active Record's tests so there's probably a place where we use nested travels 😁 |
That is exactly what broke the tests on Active Record. Preferred way is to not do that. |
as this can lead to confusing time stubbing.
Instead of:
travel_to 2.days.from_now do
# 2 days from today
travel_to 3.days.from_now do
# 5 days from today
end
end
preferred way to achieve above is:
travel_to 2.days.from_now
# 2 days from today
travel_back
travel_to 5.days.from_now
# 5 days from today
Closes rails#24690
Fixes rails#24689
vipulnsward
force-pushed
the
travel-to-raise
branch
from
4eed2a7 to
919e705
Compare
|
Nice! ❤️ |
3 participants
r? @rafaelfranca