-
Notifications
You must be signed in to change notification settings - Fork 112
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
Fix flaky IdentityLinker test #10807
Conversation
changelog: Internal, Continuous Integration, Fix flaky IdentityLinker test
* Fix flaky IdentityLinker test changelog: Internal, Continuous Integration, Fix flaky IdentityLinker test * fix extraneous "is" in test description
it 'does not set the timestamp' do | ||
freeze_time |
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.
Does this risk bleeding into tests run after this one? In other tests we use around
like this, to ensure the teardown:
around do |example|
feeze_time { example.run }
end
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.
The teardown here should take care of it: https://api.rubyonrails.org/v7.1.1/classes/ActiveSupport/Testing/TimeHelpers.html#method-i-after_teardown
I did some testing and it seems to be called after every test.
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.
Interesting. It definitely seems easier to just have the one line of code, though generally speaking I feel a little uneasy about any sort of side-effecty code like this inside a test case.
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.
I was hoping to have the freeze_time
block around it, but the helper doesn't like nested time traveling blocks:
RuntimeError:
Calling `travel_to` with a block, when we have previously already made a call to `travel_to`, 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 2.days do
# 2 days from today
end
travel 5.days do
# 5 days from today
end
🛠 Summary of changes
I got a test failure this morning due to a time-based flakiness:
This PR freezes time for the test to make it more reliable