Skip to content

Fix flaky spec around DST changes#9512

Merged
soniaconnolly merged 6 commits intomainfrom
fix-flaky-test-european-standard-time
Nov 2, 2023
Merged

Fix flaky spec around DST changes#9512
soniaconnolly merged 6 commits intomainfrom
fix-flaky-test-european-standard-time

Conversation

@soniaconnolly
Copy link
Contributor

@soniaconnolly soniaconnolly commented Nov 1, 2023

We use Zonebie so we are impacted by Europe's changes into and out of Daylight Savings Time as well as ours. Days sometimes have 23 or 25 hours, so account for that in specs by counting time within(60) minutes.

Note that we are more than 3 days out from Europe's DST change now, so this test will no longer fail with European time zones - but it would have started failing next week with North American time zones.

Auxilliary note: I would like to make Zonebie print the timezone it is using for our specs to ease debugging of such issues.

We use Zonebie so we are impacted by Europe's changes into and out of Daylight Savings Time
as well as ours. Days sometimes have 23 or 25 hours, so account for that in specs by
counting time within(60) minutes.

Co-authored-by: Jack Ryan <jack.ryan@gsa.gov>
@aduth
Copy link
Contributor

aduth commented Nov 1, 2023

As an alternative, could we freeze time to a specific point in time that we don't have to deal with changing DST?

Personally I find be_within to be a bit of a code smell, though this is less offensive to me than the ones we have that try to account for drift in milliseconds.

@mitchellhenke
Copy link
Contributor

I'd agree. The test can be made to fail reliably with:

--- a/spec/jobs/get_usps_proofing_results_job_spec.rb
+++ b/spec/jobs/get_usps_proofing_results_job_spec.rb
@@ -47,6 +47,11 @@ RSpec.shared_examples 'enrollment_with_a_status_update' do |passed:,
     end
   end

+  before do
+    Time.zone = 'Europe/Vienna'
+    allow(Time.zone).to receive(:now).and_return(Time.zone.parse('2023-11-01T00:20:00Z'))
+  end
+


it 'logs a message with common attributes' do
freeze_time do
travel_to Time.zone.parse('2023-11-30T10:00:00Z') do
Copy link
Contributor

Choose a reason for hiding this comment

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

I am generally no fan of comments, but I'll suggest either a comment here to explain why we are going to 11/30 or a method that describes that behavior, e.g.

it 'logs a message with common attributes' do
  travel_to_stable_daylight_savings_period do
    # ...
  end
end

# ...

def travel_to_stable_daylight_savings_period
  travel_to Time.zone.parse('2023-11-30T10:00:00Z') { yield }
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an explanatory variable in 8522000

Copy link
Contributor

@JackRyan1989 JackRyan1989 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @soniaconnolly

@soniaconnolly soniaconnolly merged commit d5f82fb into main Nov 2, 2023
@soniaconnolly soniaconnolly deleted the fix-flaky-test-european-standard-time branch November 2, 2023 17:50
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.

5 participants