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

Allow time to be frozen #55

Merged
merged 1 commit into from
Jun 23, 2020
Merged

Conversation

sos4nt
Copy link
Contributor

@sos4nt sos4nt commented May 26, 2020

SitePrism's wait_until_true won't work when freezing time via Timecop.freeze or Rails' travel_to. If you try to do so, a FrozenInTimeError will be raised.

This commit removes the Time#now dependency and utilizes sleep instead, which doesn't have such restrictions.

@sos4nt sos4nt force-pushed the allow_frozen_time branch 2 times, most recently from 95fe7a2 to 8eac246 Compare May 26, 2020 11:38
@sos4nt
Copy link
Contributor Author

sos4nt commented May 26, 2020

Travis shows one minor failure:

  Scenario: Load a Page that has load validations set # features/navigation.feature:32
    When I navigate a page with load validations      # features/step_definitions/navigation_steps.rb:65
    Then I am made to wait to continue                # features/step_definitions/navigation_steps.rb:80
      expected: > 0.2
           got:   0.195541809 (RSpec::Expectations::ExpectationNotMetError)

I assume this is because the new wait_until_true returns slightly faster. Maybe you could take a look into it and adjust the threshold accordingly.

@luke-hill
Copy link
Collaborator

This is going to take a bit of time to look into. As I remember distinctly the person who fixed this showed that it helped things a bit.

Undoing / altering this change feels a bit scary and I don't really use Timecop much. If you want to help / offer some more indepth stuff, that might help get this merged quickly.

As for the timeouts, don't worry about those. There are 3-5 tests that are flaky, I do mean to fix them up, but I'm lazy and I've not dedicated enough time to this project in the last 2-3 months. I need to get back on board!

In terms of "accepting" it though, it seems ok enough, just want to make sure we alter it in a way that won't nark it for others.

Copy link
Collaborator

@luke-hill luke-hill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code style tweaks:
Remove in-line disabling of cops (I want to get all the cops fixed up, again another chore for me) - See if you can refine the logic to not peeve off rubocop.
Add changelog entry. Mention you are refining existing logic to allow Rails users to use travel_to.

@sos4nt sos4nt force-pushed the allow_frozen_time branch from 8eac246 to 0083dea Compare June 4, 2020 10:47
@sos4nt
Copy link
Contributor Author

sos4nt commented Jun 4, 2020

See if you can refine the logic to not peeve off rubocop.

I've extracted the whole timer thread handling into a separate Timer class which greatly simplified the wait_until_true method.

Add changelog entry.

Done.

If you want to help / offer some more indepth stuff, that might help get this merged quickly.

The former approach polled Time.now in a loop to calculate if the method's runtime exceeded wait_time seconds:

loop do
  return true if yield
  break if Time.now - start_time > wait_time
  # ...
end     

This obviously doesn't work if Time.now is stubbed in a way that it returns the same value every time. (the way travel_to and Timecop.freeze work)

In my new approach, I'm starting a separate timer thread. The thread just sleeps for wait_time seconds and then sets a done flag:

@thread = Thread.start do
  sleep wait_time
  @done = true
end

The main thread uses a loop, just like before. But instead of checking Time.now, it now checks the timer's done flag:

loop do
  return true if yield
  break if timer.done?
  # ...
end

The huge difference is that sleep doesn't rely on Ruby's Time class. It uses C functions to suspend the timer thread for the given duration.

Note that the timer thread just sets the done flag. It doesn't interrupt the main thread in any way.

@sos4nt sos4nt force-pushed the allow_frozen_time branch from 0083dea to 28a1b98 Compare June 4, 2020 12:23
@sos4nt
Copy link
Contributor Author

sos4nt commented Jun 4, 2020

FYI: I'm going to push some minor changes (the timer spec lacks some cleanup)

@sos4nt sos4nt force-pushed the allow_frozen_time branch 2 times, most recently from f15d8e5 to b738ed9 Compare June 4, 2020 13:16
@sos4nt
Copy link
Contributor Author

sos4nt commented Jun 4, 2020

Spec is fixed now. I've also rebased the PR.

@ineverov
Copy link
Contributor

ineverov commented Jun 7, 2020

@sos4nt @luke-hill We can just replace Time.now with Process.clock_gettime(Process::CLOCK_MONOTONIC) (ruby 2.1+) and avoid a bunch of refactoring. What do you think?

@sos4nt
Copy link
Contributor Author

sos4nt commented Jun 8, 2020

@ineverov to me, using sleep looks cleaner than polling the current time.

It might also be safer than using a method that Timecop might mock in the future: travisjeffery/timecop#220

Copy link
Collaborator

@luke-hill luke-hill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for using one method over the other, I'm kind of agnostic, this enhances the gem so I'm fine with it as is, and if someone comes along later we can tweak it further.

lib/site_prism/waiter.rb Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@sos4nt sos4nt force-pushed the allow_frozen_time branch from b738ed9 to e90ce22 Compare June 19, 2020 12:17
@luke-hill luke-hill merged commit ebdda19 into site-prism:master Jun 23, 2020
@temochka
Copy link

@luke-hill would it be possible to cut a new gem version that includes this fix please? 🙏

@luke-hill
Copy link
Collaborator

@temochka One will be cut in rubygems in the next 24hours.

@temochka
Copy link

Thank you @luke-hill! This is great news 🙌

@sos4nt sos4nt deleted the allow_frozen_time branch August 25, 2020 08:10
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 this pull request may close these issues.

4 participants