Fix today's booths for officer (fixes time-related flaky specs) #1625
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a4477af to
b6c16a8
Compare
b6c16a8 to
0054ff1
Compare
Using `Date.today` generates the date on the machine the application is
being executed, and using `Date.current` generates the date defined in
`Rails.application.config.time_zone`.
Usually we don't notice the difference because most developers use the
same time zone. However, other forks (and Travis) might notice issues
once in a while. For example, this code in `Officing::BaseController`:
@officer_assignments ||= current_user.poll_officer.officer_assignments.
where(date: Time.current.to_date)
(...)
officer.officer_assignments.by_date(Date.today).map(&:booth).uniq
These lines use different dates, resulting in unexpected results. Using
`Date.current` in both of them (as the rest of the application does)
solves the problem.
In order to reproduce this problem locally, run the specs like
`TZ=Australia/Canberra rspec`, replacing Australia/Canberra to whatever
time zone where `Date.today` is different than the date in Madrid.
Using `Date.current` in both places solves the problem.
This required changing the `voted_before_sign_in` slightly in order to change what the method returns if the user signed in and voted at the exact same microsecond. It doesn't affect production code because it would be impossible for the user to do both things at the same time. As a side effect, the method now returns what the method name suggests. Before this change, the correct method name would have been `voted_before_or_at_the_same_time_of_sign_in`. As a less desirable side effect, in the tests now we need to make sure at least one second passes between the moment a user votes and the moment a user signs in again. One microsecond wouldn't work because the method `travel_to` automatically sets microseconds to zero in order to avoid rounding issues.
It was failing when executed right before midnight due to today's officer assigments changing during the test.
It was failing when executed right before midnight due to today's officer assigments changing during the test.
This way we can easily add a test which will fail if by accident we change the method to use `Date.today`. Until now using `Date.today` would only fail if we ran specs in a time zone with a different date.
Most factories were using dynamic times and dates since commit 0cf799a. However: * That commit was applied to CONSUL's main repository. At the time, commits 71f5351 and a476a30 (which introduced static times/dates in factories) hadn't been backported. * The changes in commit 0cf799a overlooked methods like `Time.zone` or `months.ago`, as well as the factory `proposal_notification`. * Commit 3bcd091 also introduced static times/dates in PR consuldemocracy#983, which hasn't been backported.
It was failing when executed right before midnight due to the date changing between the moment the notification is created and the moment the test checks the notification shows the current date.
They were failing if executed right before midnight. If the process is created right before midnight and then the date changes, when we visit the process path the phase will aready be open.
Using `Date.today` caused some milestones to be published before/after the date defined by `Rails.application.config.time_zone`. See also commit 088c76d for a more detailed explanation.
0054ff1 to
8349117
Compare
This was referenced Sep 19, 2018
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Timecopto force the date to be 01:00AM)Timecopto force the date to be 01:00AM)Background
We've been struggling with failing tests around midnight for a long time. Pull Request consul#2712 solved that problem for most of the affected specs by freezing time and using dynamic factories.
However, while those changes made results consistent on machines with the same time zone as Madrid, they were still inconsistent in places with differet time zones, like some of our forks or the machine Travis uses to run the builds.
Objectives
Fix the flaky specs that appeared in
spec/features/polls/voter_spec.rb:172("Voter Origin Trying to vote the same poll in booth and web Trying to vote in booth and then in web"),spec/features/officing_spec.rb:130("Poll Officing Officing dashboard available for multiple sessions"),spec/features/officing/booth_spec.rb:38("Booth Officer with multiple booth assignments today"),spec/features/officing/booth_spec.rb:63("Booth Display single booth for any number of polls"),spec/features/officing/booth_spec.rb:22("Booth Officer with single booth assignment today"),spec/features/officing/residence_spec.rb:120("Residence Assigned officers Error on Census (year of birth)"),spec/features/officing/residence_spec.rb:59("Residence Assigned officers Document number is copied from the census API"),spec/features/officing/residence_spec.rb:93("Residence Assigned officers Error on Census (document number)"),spec/features/officing/residence_spec.rb:40("Residence Assigned officers Verify voter"),spec/features/officing/residence_spec.rb:81("Residence Assigned officers Error on verify"),spec/features/officing/residence_spec.rb:133("Residence Verify booth"),spec/features/budget_polls/ballot_sheets_spec.rb:61("Poll budget ballot sheets Booth assignment Access ballot sheets officing with multiple booth assignments"),spec/features/budgets/executions_spec.rb:147("Executions Filters by milestone status"),spec/features/admin/admin_notifications_spec.rb:37("Admin Notifications Index Valid Admin Notifications"),spec/features/legislation/processes_spec.rb:180("Legislation process page draft publication phase not open"),spec/features/legislation/processes_spec.rb:201("Legislation process page allegations phase not open"),spec/features/legislation/processes_spec.rb:222("Legislation process page final version publication phase not open"),spec/features/legislation/processes_spec.rb:144("Legislation process page debate phase not open"),spec/features/legislation/processes_spec.rb:243("Legislation process page proposals phase not open").Explain why the test is flaky, or under which conditions/scenario it fails randomly
As explained in the commit message:
Explain why your PR fixes it
Using
Date.currentin both of them (as the rest of the application does, see for example commit e14a5b2 ) solves the problem.Notes
PR Budget execution list #1543(already backported) and maybe others.There might be conflicts with consul#2838 as wellConflicts because we split factories are already solved.TODO: we can now backport the part related to consul#2945.Already backportedTZ=Australia/Canberra rspec, replacing "Australia/Canberra" to whatever time zone whereDate.todayis different than the date in Madrid.todays_booths_for_offcercode, and all of them pass after changing it no matter what combination of time and time zone we're in. Therefore, AFAIK this PR (finally!) closes all issues mentioned in the references section. I could be wrong, though, and there might be yet another timing issue we haven't found so far.Date.today.todays_booths_for_officerfixed behaviour affecting the development and production environments. If these changes should be done in a different pull request, please let me know.