porta
porta copied to clipboard
3scale
[Threescale 8027] Replace Timecop with ActiveSupport::Testing::TimeHelpers
Which issue(s) this PR fixes
Special notes for your reviewer:
We can't get completely rid of Timecop, because there's no replacement for Timecop.scale, which is called from two callbacks which are executed from many tests:
- https://github.com/jlledom/porta/blob/THREESCALE-8027/features/support/hooks.rb#L67
- https://github.com/jlledom/porta/blob/THREESCALE-8027/spec/support/api/context.rb#L7
As far as I know, there's no replacement or possible workaround to get rid of these calls, so as I see it, we can only proceed with one of the next two options:
- Migrate everything to Rails TimeHelpers, except those three calls to
Timecop.scale, which can create confusion since we'll be using two libraries for doing the same thing. - Give up and don't migrate anything, which will keep us using an unmaintained library forever.
I think this is the first time I ever saw Timecop.scale being used on tests 😅. Do you think we really need it?
From the two files you shared, I'm actually failing to understand why the speed up on time is even necessary, as we don't seem to be testing/asserting any time-related value.
Maybe (just maybe) we're able to remote it or even add something like travel(1.second) on tests that really need it, which would make things more explicit.
I think this is the first time I ever saw Timecop.scale being used on tests 😅. Do you think we really need it? From the two files you shared, I'm actually failing to understand why the speed up on time is even necessary, as we don't seem to be testing/asserting any time-related value. Maybe (just maybe) we're able to remote it or even add something like
travel(1.second)on tests that really need it, which would make things more explicit.
We would need to add time to every update test which would probably slow down the tests too much, as @jlledom pointed me out in a private chat.
However, I do think it must be possible to remove scale somehow. Maybe making a different assertion. After all, testing updated_at changed seems to be like testing internal rails code which should be out of our responsibility.
However, I do think it must be possible to remove scale somehow. Maybe making a different assertion. After all, testing updated_at changed seems to be like testing internal rails code which should be out of our responsibility.
Agree, I'll investigate further in the old repo to figure out why are those tests there.
I just saw the thread about this and thought about Dirty#saved_changes?. I didn't try it but it might work if we remove that reload as well 🤔
tbh testing updated_at seems to be strange - testing rails internals...
It's not testing Rails internals, but relying on Rails internals to perform a higher level assertion. I'll explain more in my next comment.
Rails TimeHelpers don't include all the functionality provided by Timecop:
| Timecop | TimeHelpers | Comment |
|---|---|---|
freeze |
freeze_time |
Freezes Time.now at now |
freeze(time) |
travel_to(time) |
Freezes Time.now at a given time |
return |
unfreeze_time, travel_back |
Time.now back to normal |
travel(time) |
❌ | Moves to the given time but doesn't freeze Time.now, so time continues passing |
| ❌ | travel(offset) |
Freezes Time.now to now + given offset |
scale(factor) |
❌ | Makes time pass faster by the given factor |
Replacing Timecop.scale is difficult because there are some tests that depends on the time passing faster than normal. For instance, the test at:
spec/support/api/crud.rb#L23-L26
This asserts that any time a CRUD #update controller is called over a resource (PUT /path/to/:resource_id) the resource is in fact updated. It relies on the Rails internal logic which updates the updated_at attribute when the resource is saved.
Since our DB schema is configured to not use decimals in timestamps, the precision in those columns is 1 second, so when update is called less than a second after the creation, the new updated_at value is the same. And since messing with DBs is not desirable, Timecop.scale is called to accelerate the pace of time, here:
spec/support/api/context.rb#L7
It makes one hour pass in the test for every second in real life, so even when update is called a few milliseconds after the creation, more than a minute passed in the test, and thus update_at gets a different value and the test pass.
To workaround this problem, I manually set the attributes updated_at and created_at to one second before their original value, so when the Rails method updates the updated_at column, it won't be equal to created_at, even in the case the resource was created and then updated in the same second:
spec/support/api/crud.rb#L23-L27
On the other hand, there are many cucumbers which also need time passing faster than normal, look at:
features/support/hooks.rb#L67-L69
A couple of tests for provider's applications search failed due to a wrong described context:
features/old/applications/providers/search.feature#L19-L22
That step creates two applications at the same second, and since the search results are ordered by creation time, that breaks the order test. It was working before because the time was passing 100 times faster, so even a difference of milliseconds between creating the two applications was enough to generate different timestamps.
I solved this by refactoring the way those applications are created to ensure they generate different timestamps.
features/old/applications/providers/search.feature#L17-L21
About all other cucumbers, They are fixed by updating the time_helpers and time_machine steps:
features/support/time_machine_helpers.rb
features/step_definitions/timehelpers_steps.rb
Added a couple of nitpicks. I think we can ignore the "Utility Function" warning - we discussed that before, it doesn't make much sense in tests.
Otherwise - great job! :clap:
