search

travisjeffery / timecop

A gem providing "time travel", "time freezing", and "time acceleration" capabilities, making it simple to test time-dependent code. It provides a unified method to mock Time.now, Date.today, and DateTime.now in a single call.
MIT License
3.36k stars 223 forks source link

Fix `frozen?` when scaled or traveling #387

Closed jamiemccarthy closed 1 year ago

jamiemccarthy commented 2 years ago

While fine-tuning an application's use of Timecop, I noticed that frozen? was returning true if any mock type was active, not just :freeze but also :travel or :scale.

I believe this was not the intended meaning of the method, neither according to its name nor as described in the class's code.

This PR updates frozen? so it returns true only if there is at least one mock type on the stack and the last (currently active) mock type is in fact :frozen.

I ran the test suite in the Dockerfile, but I haven't run it against the other ruby's in the CI matrix.

The first commit is unrelated and just adds a few more tests that I thought might be useful. If you want to skip it, that's fine. The other three commits add tests, fix the method, and update the History, respectively.

Comments and questions welcome, and if you'd like me to refactor this PR in any way I'm happy to! Thanks for writing this very useful gem!

joshuacronemeyer commented 1 year ago

@jamiemccarthy this seems like a nice PR. Thank you. When you get the chance can you change it to not use safe navigation? For older ruby compatibility we can't use that.

jamiemccarthy commented 1 year ago

@jamiemccarthy this seems like a nice PR. Thank you. When you get the chance can you change it to not use safe navigation? For older ruby compatibility we can't use that.

Sure thing, sorry I missed that in the first place!

jamiemccarthy commented 1 year ago

@joshuacronemeyer Let me know if there's anything else I can do!

joshuacronemeyer commented 1 year ago

@jamiemccarthy thanks for the reminder. Would you mind getting your branch up to date with master? I'm happy to merge this when that's done and tests pass.

jamiemccarthy commented 1 year ago

Hey there @joshuacronemeyer, you merged in master (f49cea095d3a) already last month, and I see a green check on d274f02360f2b77b4f7611797c40c3bcf448d39c, so I think we're good to go!

joshuacronemeyer commented 1 year ago

Hey there @joshuacronemeyer, you merged in master (f49cea0) already last month, and I see a green check on d274f02, so I think we're good to go!

I'll take a look soon then. Github UI says this branch can't be rebased due to conflicts.

jamiemccarthy commented 1 year ago

I see, yes -- it looks like my addition at the top of History.md collides with the changelog for 0.9.6. It will merge cleanly but a rebase will require resolving that manually.

I've rebased and resolved that as a new branch, https://github.com/jamiemccarthy/timecop/tree/jm-frozen-check-rebased , which you're welcome to use. If you'd like me to force-push that to this PR, just let me know.