Fix flaky specs: Officing Results Add/Edit results #2712

javierv · 2018-07-01T19:28:16Z

References

Issue Flaky spec: Officing Results Edit result #2520
Issue Flaky spec: Officing Results Add results #2521
Issue Flaky spec: Unable to find visible select box "residence_document_type" that is not disabled AyuntamientoMadrid/consul#1209
Pull Request Flaky spec: Voter Origin Trying to vote in booth and then in web AyuntamientoMadrid/consul#1342
Pull Request Flaky spec: Polls Booth & Website Already voted on booth cannot vote on website AyuntamientoMadrid/consul#1349
~~Pull Request Flaky spec: Residence Verify booth Unable to find visible css "#officing-booth" AyuntamientoMadrid/consul#1351~~ Forked repo build with failed residence specs

Objectives

Fix the flaky specs that appeared in spec/features/officing/results_spec.rb:48 ("Officing Results Add results"), spec/features/officing/results_spec.rb:85 ("Officing Results Edit result"), spec/features/officing/residence_spec.rb:35 ("Residence Assigned officers Verify voter"), spec/features/officing/residence_spec.rb:83 ("Residence Assigned officers Error on Census (year of birth)"), spec/features/officing/residence_spec.rb:49 ("Residence Assigned officers Error on verify") and spec/features/officing/residence_spec.rb:61 ("Residence Assigned officers Error on Census (document number)").

Explain why the test is flaky, or under which conditions/scenario it fails randomly

This explanation is copied from AyuntamientoMadrid#1342 because the problem is exactly the same. All credit to @iagirre.

The problems was that the test was not reaching to the residence verification page, so it couldn't find the select box for the DNI (and that's the error reported in the issue). To figure out what was going on there, [she] studied the flow that the test follows so that [she] could find the scenarios where it would crash, and, after doing it, step by step, [she] determined that the test will fail when:

There are more than 1 officer_assignments (booths) for this user, so the page will redirect to the select booth page, instead to the residence verification page.
There aren't any officer_assignments, so that the user can't even reach the target page (it redirects to the root).

The code that determines that is (this functions are run in before_action blocks):

# app/controller/officing/base_controller.rb
# [...]

  def load_officer_assignment
    @officer_assignments ||= current_user.poll_officer.officer_assignments.where(date: Time.current.to_date)
    end

    def verify_officer_assignment
      if @officer_assignments.blank?
        redirect_to officing_root_path, notice: t("officing.residence.flash.not_allowed")
      end
    end

    def verify_booth
      return unless current_booth.blank?
      booths = todays_booths_for_officer(current_user.poll_officer)
      case booths.count
      when 0
        redirect_to officing_root_path
      when 1
        session[:booth_id] = booths.first.id
      else
        redirect_to new_officing_booth_path
      end
    end

    [...]

    def todays_booths_for_officer(officer)
      officer.officer_assignments.by_date(Date.today).map(&:booth).uniq
    end

After trying to reproduce it (without success), [she] saw the new piece of information about the probable failing hours, and [she] realized that the problem could be in the moment of creating the objects. If the officer_assignments are created at 23:59:59 and the rest of the test is executed after 00:00:00, the dates for the objects and the Date.current (used to check if there are any shifts today) won't be the same, because the shift will be for, lets say, 07/03/2018 and Date.current will be 08/03/2018, so, there are no shifts and we find the first scenario described above.

To prove that, [she] forced in the factories.rb the dates of the officer_assignments to an earlier date, and it failed in the exact same way as the reported one.

Explain why your PR fixes it

Freezing time ensures the date won't change in the middle of a test, and using dynamic attributes for dates ensures factories don't assign static dates before freezing time. According to the factory bot documentation:

Most factory attributes can be added using static values that are evaluated when the factory is defined, but some attributes (such as associations and other attributes that must be dynamically generated) will need values assigned each time an instance is generated. These "dynamic" attributes can be added by passing a block instead of a parameter:
(...)
date_of_birth { 21.years.ago }

Notes

I wasn't sure whether to use the same solution used in Flaky spec: Voter Origin Trying to vote in booth and then in web AyuntamientoMadrid/consul#1342 or to add Timecop to the Gemfile, but then realized Rails provides helpers to do exactly that and thought it was the easiest way. This decision is of course debatable; comments are welcome!
Issues Flaky spec: Voter Origin Trying to vote in booth and then in web AyuntamientoMadrid/consul#1195 and Flaky spec: Polls Booth & Website Already voted on booth cannot vote on website AyuntamientoMadrid/consul#1208 are similar and their fixes (referenced above) haven't been backported yet.
After upgrading to Rails 5, monkey-patching ActiveSupport::Testing::TimeHelpers won't be necessary. The after { travel_back } block won't be necessary either.

aitbw

Hi @javierv! Thanks a lot for your latest contributions to CONSUL 🎉

I was wondering if is it possible to implement a solution similar to those found in the PRs you mentioned 🤔

Monkey patching is not really my cup of tea 😅 and since we do plan on migrating to Rails 5 (hopefully very soon 🤞 —see #2631, #2627 and #2628), I think it'd be better to keep CONSUL's codebase as clean as possible and refactor if necessary when we jump into the Rails 5 bandwagon 😸

Let me know what you think!

javierv · 2018-07-02T16:34:20Z

Hi @aitbw. Thank you for your comments 👍! I've removed the monkey patch.

I was wondering if is it possible to implement a solution similar to those found in the PRs you mentioned.

Would you mind helping me with that? I'm not familiar with the source code and even after checking the other PRs am not sure which factories need to be changed 🙀.

aitbw · 2018-07-02T21:08:08Z

🤔 You added a new solution on the latest commit. Does this solves the flakies as well? @javierv

Because if that's the case, I don't think we'd need to implement something similar to the PRs you mentioned

javierv · 2018-07-02T23:23:39Z

@aitbw Yes, the latest commit only refactors the code, doing the same thing without monkey patching. According to what I've tested locally, it solves the flakies (EDIT: but since my tests were completely missing the point, it didn't solve the flakies). But it would be great to have another pair of eyes have a look 😉.

javierv · 2018-07-04T18:40:37Z

@aitbw Do you think it makes sense to backport the mentioned pull requests in order to complete this one, since they deal with similar flakies?

P.S. (Sorry for the off-topic) I've just sent you an email. Hope it's OK 😰! If you haven't received it, it might be inside your spam folder.

javierv · 2018-07-06T00:28:28Z

@aitbw I've done some more testing and I've realized I hadn't fixed anything 😅. I had forgotten to make sure factories were assigning dates dynamically.

I've added another commit and tested it better, both manually and using Timecop, and it looks like this time the flakies are actually fixed. Somehow I hadn't realized until now that by changing my computer's date this can be tested anytime, and had also forgotten about Timecop.travel 😲. Sorry for that!

javierv · 2018-07-06T02:04:35Z

I've realized AyuntamientoMadrid#1351 only fixes one test in spec/features/officing/residence_spec.rb, and that test isn't present on this repo. So I've added another commit fixing the flakies in spec/features/officing/residence_spec.rb, which failed on a recent build on my forked repo:

1) Residence Assigned officers Verify voter
     Failure/Error: select 'DNI', from: 'residence_document_type'
     
     Capybara::ElementNotFound:
       Unable to find visible select box "residence_document_type" that is not disabled
     # ./spec/features/officing/residence_spec.rb:40:in `block (3 levels) in <top (required)>'
  2) Residence Assigned officers Error on Census (year of birth)
     Failure/Error: select 'DNI', from: 'residence_document_type'
     
     Capybara::ElementNotFound:
       Unable to find visible select box "residence_document_type" that is not disabled
     # ./spec/features/officing/residence_spec.rb:88:in `block (3 levels) in <top (required)>'
  3) Residence Assigned officers Error on verify
     Failure/Error:
       within("#new_residence") do
         click_button "Validate document"
       end
     
     Capybara::ElementNotFound:
       Unable to find visible css "#new_residence"
     # ./spec/features/officing/residence_spec.rb:54:in `block (3 levels) in <top (required)>'
  4) Residence Assigned officers Error on Census (document number)
     Failure/Error: select 'DNI', from: 'residence_document_type'
     
     Capybara::ElementNotFound:
       Unable to find visible select box "residence_document_type" that is not disabled
     # ./spec/features/officing/residence_spec.rb:67:in `block (3 levels) in <top (required)>'

AyuntamientoMadrid#1343 also fixes tests which aren't present on this repo. So only two pull requests would need to be backported. I've updated the description of this pull request accordingly.

aitbw · 2018-07-08T21:15:34Z

spec/features/officing/results_spec.rb

@@ -158,4 +159,7 @@
    within('#total_results') { expect(page).to have_content('66') }
  end

+  after do


Can you move this block right after the background block? It's easier to understand what's going to happen that way 😸

aitbw · 2018-07-08T21:16:28Z

spec/features/officing/residence_spec.rb

@@ -96,4 +100,7 @@

  end

+  after do


Can you move this block right after the background block? It's easier to understand what's going to happen that way 😸

Amend if you can, there's no need to create a new commit for this.

aitbw · 2018-07-08T21:46:35Z

Hello once again @javierv, thanks big time for this PR 🎉

As usual, I'll be testing this thoroughly locally and on Travis to see how well it fares but seems like it'll do 😸

Re: the tests found on Ayuntamiento Madrid but not here —we have some custom functionality over there we don't plan to backport. Also, there are flakies which you'll only find on Ayuntamiento Madrid so that's why some specs, while they do test the same scenarios, will differ.

@iagirre

As explained by @iagirre with pinpoint accuracy [1]: "If the officer_assignments are created at 23:59:59 and the rest of the test is executed after 00:00:00, the dates for the objects and the `Date.current` (used to check if there are any shifts today) won't be the same, because the shift will be for, lets say, 07/03/2018 and `Date.current` will be 08/03/2018, so, there are no shifts." Freezing time avoids this issue. Related to issues consuldemocracy#2520 and consuldemocracy#2521. [1] AyuntamientoMadrid#1342

@aitbw

It could have side effects (for example, a conflict after upgrading to Rails 5). Thanks @aitbw for the suggestion!

The tests depending on the date changing were still failing because Date.current was being stubbed after loading the factories. The following lines affected these specific tests: factory :poll_officer_assignment, class: 'Poll::OfficerAssignment' do (...) date Date.current end So if the tests were executed right before midnight, the sequence was: 1. The factories file was loaded, assigning Date.current to the date of every Poll::OfficerAssignment to be created. 2. Time passed, so now it was after midnight. 3. The `travel_to` method freezed time, after midnight. 4. A Poll::OfficerAssignment factory was created, using the date it was before midnight. Using dynamic fixtures solves the problem: factory :poll_officer_assignment, class: 'Poll::OfficerAssignment' do (...) date { Date.current } end Now the sequence is: 1. The factories file is loaded, and since it finds a block, doesn't assign a static value to every Poll::OfficerAssignment to be created. 2. Time passes, so now it's after midnight. 3. The `travel_to` method freezes time, after midnight. 4. A Poll::OfficerAssignment factory was created, and in executes the block, using the current date, that is, after midnight.

As it happened with results (commit 4a559ed), these tests failed if `Date.current` changes between the moment records are created and the moment the rest of the test is executed.

javierv · 2018-07-08T22:23:42Z

@aitbw Thank you for your comments!

I pushed the changes quickly so Travis' build would run right before midnight. However, I made a small mistake deleting a couple of empty lines which had nothing to do with this pull request. So I've rebased again.

voodoorai2000 · 2018-07-12T14:06:23Z

Very glad you figured out how this part of the codebase works, not trivial 👏

javierv changed the title ~~Avoid date changes during results tests.~~ Fix flaky officing results specs Jul 1, 2018

aitbw suggested changes Jul 2, 2018

View reviewed changes

javierv mentioned this pull request Jul 5, 2018

Fix flaky specs: Votes Debates and Voting comments Update #2734

Merged

javierv force-pushed the 2520_and_2521-fix_flaky_results_specs branch from 09f8900 to 9285685 Compare July 6, 2018 00:31

javierv changed the title ~~Fix flaky officing results specs~~ Fix flaky specs: Officing Results Add/Edit results Jul 6, 2018

aitbw reviewed Jul 8, 2018

View reviewed changes

javierv force-pushed the 2520_and_2521-fix_flaky_results_specs branch from df0a96c to d613ac4 Compare July 8, 2018 21:52

javierv added 4 commits July 9, 2018 00:06

Don't monkey patch ActiveSupport.

de0afe1

It could have side effects (for example, a conflict after upgrading to Rails 5). Thanks @aitbw for the suggestion!

Avoid date changes during residence tests.

735eab8

As it happened with results (commit 4a559ed), these tests failed if `Date.current` changes between the moment records are created and the moment the rest of the test is executed.

javierv force-pushed the 2520_and_2521-fix_flaky_results_specs branch from d613ac4 to 735eab8 Compare July 8, 2018 22:22

voodoorai2000 merged commit 046b620 into consuldemocracy:master Jul 12, 2018

javierv mentioned this pull request Jul 12, 2018

Flaky spec: Unable to find visible select box "residence_document_type" that is not disabled AyuntamientoMadrid/consul#1209

Closed

3 tasks

javierv deleted the 2520_and_2521-fix_flaky_results_specs branch July 12, 2018 15:09

javierv mentioned this pull request Jul 13, 2018

Flaky spec: Voter Origin Trying to vote in booth and then in web AyuntamientoMadrid/consul#1195

Closed

3 tasks

decabeza mentioned this pull request Jul 16, 2018

Upstream AyuntamientoMadrid/consul#1568

Merged

This was referenced Aug 10, 2018

Flaky spec: Officing Results Edit result #2520

Closed

Flaky spec: Officing Results Add results #2521

Closed

javierm mentioned this pull request Aug 30, 2018

Fix today's booths for officer (fixes time-related flaky specs) AyuntamientoMadrid/consul#1625

Merged

javierm mentioned this pull request Sep 19, 2018

Flaky spec: Poll Officing Officing dashboard available for multiple sessions AyuntamientoMadrid/consul#1200

Closed

3 tasks

javierm self-assigned this Nov 11, 2021

javierm added the Flaky spec label Nov 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix flaky specs: Officing Results Add/Edit results #2712

Fix flaky specs: Officing Results Add/Edit results #2712

javierv commented Jul 1, 2018 •

edited

Loading

aitbw left a comment

javierv commented Jul 2, 2018

aitbw commented Jul 2, 2018

javierv commented Jul 2, 2018 •

edited

Loading

javierv commented Jul 4, 2018 •

edited

Loading

javierv commented Jul 6, 2018 •

edited

Loading

javierv commented Jul 6, 2018 •

edited

Loading

aitbw Jul 8, 2018

aitbw Jul 8, 2018

javierv Jul 8, 2018

aitbw commented Jul 8, 2018

javierv commented Jul 8, 2018 •

edited

Loading

voodoorai2000 commented Jul 12, 2018

Fix flaky specs: Officing Results Add/Edit results #2712

Fix flaky specs: Officing Results Add/Edit results #2712

Conversation

javierv commented Jul 1, 2018 • edited Loading

References

Objectives

Explain why the test is flaky, or under which conditions/scenario it fails randomly

Explain why your PR fixes it

Notes

aitbw left a comment

Choose a reason for hiding this comment

javierv commented Jul 2, 2018

aitbw commented Jul 2, 2018

javierv commented Jul 2, 2018 • edited Loading

javierv commented Jul 4, 2018 • edited Loading

javierv commented Jul 6, 2018 • edited Loading

javierv commented Jul 6, 2018 • edited Loading

aitbw Jul 8, 2018

Choose a reason for hiding this comment

aitbw Jul 8, 2018

Choose a reason for hiding this comment

javierv Jul 8, 2018

Choose a reason for hiding this comment

aitbw commented Jul 8, 2018

javierv commented Jul 8, 2018 • edited Loading

voodoorai2000 commented Jul 12, 2018

javierv commented Jul 1, 2018 •

edited

Loading

javierv commented Jul 2, 2018 •

edited

Loading

javierv commented Jul 4, 2018 •

edited

Loading

javierv commented Jul 6, 2018 •

edited

Loading

javierv commented Jul 6, 2018 •

edited

Loading

javierv commented Jul 8, 2018 •

edited

Loading