Skip to content

Comments

Use fixed argument to travel_to rather than Time.zone.now#8569

Merged
soniaconnolly merged 2 commits intomainfrom
friday-test-hacking/fix-delete-account-spec
Jun 9, 2023
Merged

Use fixed argument to travel_to rather than Time.zone.now#8569
soniaconnolly merged 2 commits intomainfrom
friday-test-hacking/fix-delete-account-spec

Conversation

@soniaconnolly
Copy link
Contributor

@soniaconnolly soniaconnolly commented Jun 9, 2023

Because this spec file fails around midnight UTC which is 5pm PDT.

soniaconnolly and others added 2 commits June 9, 2023 10:38
@soniaconnolly @jmax-gsa @matthinz
Use fixed argument to travel_to rather than Time.zone.now
64ac3b9 
Because tests become flaky around midnight UTC which is 5pm PDT.

Co-authored-by: John Maxwell <john.maxwell@gsa.gov>
Co-authored-by: Matt Hinz <matt.hinz@gsa.gov>
@soniaconnolly
changelog
6b6c94c 
[skip changelog]
@soniaconnolly soniaconnolly requested review from a team and olatifflexion June 9, 2023 17:48
solipet
solipet approved these changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

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

LGTM

@aduth
Copy link
Contributor

aduth commented Jun 9, 2023

I don't fully understand why this is necessary, which makes me fearful how to apply this generally. What was specifically failing, and would it be enough to either move assertions within the time traveling block or freeze time altogether?

@soniaconnolly
Copy link
Contributor Author

soniaconnolly commented Jun 9, 2023 •
edited
Loading

I don't fully understand why this is necessary, which makes me fearful how to apply this generally. What was specifically failing, and would it be enough to either move assertions within the time traveling block or freeze time altogether?

travel_to does freeze time. The problem was that it was traveling to Time.zone.now which was getting frozen very close to midnight UTC when run at ~5pm PDT, which caused the test to fail because it cares about which day it is. We thought about applying it more generally, but decided to wait and see if any other tests fail this way. The failure was "No email has been sent!"

@soniaconnolly soniaconnolly merged commit 88c8bb0 into main Jun 9, 2023
@soniaconnolly soniaconnolly deleted the friday-test-hacking/fix-delete-account-spec branch June 9, 2023 23:12
soniaconnolly added a commit that referenced this pull request Jun 12, 2023
@soniaconnolly
Revert "Use fixed argument to travel_to rather than Time.zone.now (#8569
7b3a0f9 
)"

This reverts commit 88c8bb0.
@soniaconnolly soniaconnolly mentioned this pull request Jun 12, 2023
Merged
zachmargolis pushed a commit that referenced this pull request Jun 12, 2023
@soniaconnolly
Fix specs that are broken on main (#8583)
064ecf5 
* Revert "Use fixed argument to travel_to rather than Time.zone.now (#8569)"

This reverts commit 88c8bb0.

* Add RSpec.describe instead of describe

[skip changelog]
@soniaconnolly soniaconnolly restored the friday-test-hacking/fix-delete-account-spec branch June 16, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@solipet solipet solipet approved these changes

@olatifflexion olatifflexion Awaiting requested review from olatifflexion

+1 more reviewer

@zachmargolis zachmargolis zachmargolis approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@soniaconnolly @aduth @zachmargolis @solipet