Skip to content
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

Flaky spec: Voter Origin Trying to vote in booth and then in web #1342

Conversation

iagirre
Copy link

@iagirre iagirre commented Mar 8, 2018

References

Objectives

Fix the flaky that appeared in spec/features/polls/voter_spec.rb:142.

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

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, I studied the flow that the test follows so that I could find the scenarios where it would crash, and, after doing it, step by step, I 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), I saw the new piece of information about the probable failing hours, and I 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, I 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

I stubed the Date and Time clases to force them to give me a date I can control. I set that date (01/01/2018) to the objects that depend on it (poll_shift, officer_assignment and polls). This way, it doesn't matter when the test is executed because for him it will always be the same date.

Thank you to @raul-fuentes for reminding me about the stubs and mocks 🎉

Visual Changes (if any)

There aren't, is a flaky.

Notes

  • I added stubs for Date.current, Date.today and Time.current (this are all the functions used to check the shifts and officer assignments) so that they all return a predefined date: 01/01/2018.
  • I stubed them in a before block, so that it works for all the test.
  • I set that date to the objects that depend on it (poll_shift, officer_assignment and polls). Take into account that polls have not a Date, they have a range (defined by starts_at and ends_at), so I set the start_at at 2017/12/01 and the ends_at at 2018/02/01.

@voodoorai2000
Copy link

Excellent @iagirre 😌
Will run the build after midnight to doble check, but it looks like the right solution 👍

background do
create(:geozone, :in_census)
create(:poll_shift, officer: officer, booth: booth, date: Date.current, task: :vote_collection)
create(:poll_shift, officer: officer, booth: booth, date: "2018-01-01", task: :vote_collection)

Choose a reason for hiding this comment

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

Couldn't the Date.current still be used to set the date attribute?

Copy link
Author

Choose a reason for hiding this comment

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

Definitely yes, I didn't realised before. I've changed it.

Copy link

@MariaCheca MariaCheca left a comment

Choose a reason for hiding this comment

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

Good! I just left a comment for you to see as soon as you can :)

…e. This avoids the dependance in the time the test is run, because for him it will always be the same day.
@iagirre iagirre force-pushed the 1195-flaky-voter-origin-trying-to-vote-in-booth-and-then-in-web branch from 3f191bf to 35d011c Compare March 14, 2018 07:39
@MariaCheca MariaCheca merged commit 2e13d44 into AyuntamientoMadrid:master Mar 14, 2018
@aitbw aitbw deleted the 1195-flaky-voter-origin-trying-to-vote-in-booth-and-then-in-web branch April 5, 2018 15:15
javierv added a commit to javierv/consul that referenced this pull request Jul 1, 2018
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
javierv added a commit to javierv/consul that referenced this pull request Jul 8, 2018
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
clairezed added a commit to CDJ11/CDJ that referenced this pull request Aug 4, 2018
* Docker/docker-compose enhancements

The following enhancements have been made to docker/docker-compose

* Fixed bug when building the image.
* docker-compose up starts the server
* Scaffolding inside the container respect the ownership of the files
outside it
* Volumes are tagged as 'delegated' in order to improve performance for
mac/windoze users.
* bundler stores packages in a volume. This whay new packages can be
added without rebuilding the image:

```bash
docker-compose run app bundle install
```

* Adds missing label for header card

* Fixes styles for admin homepage

* Expands proposals content if there no have image

* Ignores relative to the editor moved from gitignore to gitignore_global

* Split spec common actions support helper

* Clean up newly created modules

* Updates foundation gem

* Adds foundation assets

* Adds custom consul settings

* Updates top bar html and styles

* Updates admin top bar html and styles

* Fixes lint sccs warnings

*  Updates tabs component for proposals

* Updates tabs component for budget investments

* Updates tabs component for legislation proposals

* Updates top bar class on specs

* Updates active to is-active class for menus

* Adds border radius on pagination

* Avoids duplicated border on cards

* Adds missing tablist and tab roles

* Fixes styles for menu icon, button on header and figure img

* Adds version to foundation and autoprefixer gems

* Updates all active to is-active classes

* Fixes scss lint warnings

* Removes custom content on debates index

* Fixes headings height on budgets index

* Adds missing i18n on welcome recommendations

* Moves mailer header and footer to partials

This prevent use the entire HTML layout, an HTML inside another HTML
without using an iframe create some validation errors and break layout
details

* Shows see results button only if budget finished

* Adds see results button on budget investment admin

* Improves homepage layout

* Updates pull resquest template

* Fixes duplicated data-deep-link-smudge attribute

* Adds missing features i18n and cleans i18n-tasks

* Replaces foundation header-sizes to header-styles

* Adds missing class on proposals show

* Adds debates_path on back link to on debates new

* Prevents hide content on equalizer container

* Avoids show duplicated budget investment price

* Fixes accesskey on subnavigation

* Fixes indentation on proposals show

* Adds data-tabs on admin poll booth assignments show

* Reorders admin menu sections

* Removes pending content to backport

* Improves i18n texts

* Updates texts on specs

* Fixes tabs markup on poll booth assignments show

* Improves legislation questions form

* Removes old help on moderation dashboard

* Updates i18n of management menu

* Fixes styles of management layout header

* Improves i18n on admin notifications form

* Removes duplicated link on admin budgets groups

* Removes investments link on admin budgets heading

* Improves layout and i18n for admin newsletters

* Updates i18n texts on specs

* Removes hollow class on moderators and valuators buttons

* Adss missing th on legislation processes index table

* Moves content blocks info to index view

* Reorders and improve i18n for legislation processes form

* Shows always show to valuators on investments table

* Updates texts on admin legislation specs

* Updates specs for show to valuators checkbox

* Fix select investment specs

The select button seems to be outside the window. Removing some columns
to make the select button visible by capybara

This is just a temporary fix, we have to find a better solution.
Playing with the css to find an appropriate length for the table
containing this select button, is probably the way to go

* Restores removed columns

* Removes content pending to backport

* Fix specs

* Adding images and seeds for the homepage

* Fixes admin menu toggle

* Adds data-equalizer to fix height on homepage cards

* Replaces header and debates images

* Adds gradient overlapping card image

* Adds card hover effect

* Fixes scss lint warning

* Don't use jQuery animations in tests.

There's a flaky test creating a group for a budget which takes place
because toggling a form using jQuery sometimes results in the button not
being correctly clicked by Capybara.

Checking the page with `expect(page).to have_button 'Create group'`
before clicking the button doesn't solve the problem; it looks like in
those cases Capybara waits for AJAX requests but not for JavaScript
animations.

See also issue consuldemocracy#2573.

* Extract partial with common <head> tags.

* Skip jQuery animations in all layouts when testing.

* Don't use CSS animations when testing.

Just like jQuery animations, they can make tests fail when using
Capybara.

* Rename partial to diable animations in tests.

With this name, it's easier to understand what it does.

* Changed the way the notifications page is accessed. Instead of doing it through UI, after the notifications are created the test goes directly to the notifications page (after login in the user). To check if the notification icon is correctly shown, a new test has been created that only does that.

* Regenerate Gemfile.lock

* Fix Valuation Investment index heading filters

Why:

Heading filter where not being correctly displayed

How:

Increasing scenario to cover all possible combinations, and fixing the
heading_filters method of the Valuation Budget Investment Controller to
correctly:
  * Find how many investments the valuator can access
  * Count investments for each heading

* Group Index scenarios at Valuation Investments spec

Grouping scenarios makes it easier to follow the spec and to know where
to put a new scenario

* Add Budget::Investment::Status migration

* Add Budget::Investment::Statuses model

* Add Budget::Investment::Statuses controller and routes

* Add Budget::Investment::Statuses views

* Add new translations

* Add Budget::Investment::Statuses tests

* Add Budget::Investment::Status to dev_seeds

* Add Budget::Investment::Status migration

* Change Budget::Investment::Milestones controller

Added `:status_id` to valid params and new method to get all statuses defined for the budget.

* Changed Budget::Investment::Milestones model

Added relation with Status and set condition to validate milestone's description presence if there's no status set.

* Changed Budget::Investment::Milestone views

* Change public investment milestones timeline

* Add new translations

* Add Budget::Investment::Statuses tests

* Adds styles to budget investment milestone status and fixes spec

* Fix suggestions keyup timeout.

The browser was generating one AJAX request per keystroke, ignoring the
timeout. The clearTimeout() function needs to be called with the ID
value returned by setTimeout().

* Fix string literal warning for admin menu

* Reorganizes manager menu and creates helper

* Updates styles for management section

* Updates management i18n

* Cleans and improves management views layout

* Updates texts on specs

* Adds ballot booths menu on admin

* Add migration to generate new columns

The migration to generate the columns needed for the feature.
There are three new columns:

moderated: a boolean that, when true, it means a moderator has
hidden a proposal notification. The notification is hidden immediately
and it's shown in the moderators proposal notifications index.

hidden_at: used by acts_as_paranoid to hide the notification from the list.
It's like deleting it, but without deleting definitely from DB.

ignored_at: used to mark as a notification as ignored, so that it will
appear in marked as reviewed and not in the pending list. WARNING! this
doesn't mean that it will disappear from the 'All' filter.

* Add backend for the moderators

Add new routes for the proposal notifications edition and
abilities to let moderators edit it (mark as ignored, hide, etc.).

The notifications are not flaggable because they doesn't work like that,
but in a similar way. The moderator/administrator is in charge of hidding
them through the UI, so the normal users don't flag it as inappropriate.

New controller Moderation::ProposalNotification to manage the moderators
work.

* Add the menu entry for prop. notifications

A new menu for the sidebar has been added, so that the moderator
can access to the index from the menu.

* Add the index to moderate the notifications

Add the index for moderating the notifications. The tranlations needed
have also been added, along with the JS to make it disappear at that moment.

* Add specs to test the prop. notifications hide action

Specs that test if the proposal notifications hide action works.
It also tests if the admin part works (mark as reviewed, ignore them, etc.)

* Add hide button in the user interface

The little menu with the hide notification link and block user link
has been added to each proposal notification.

JS for adding the fade efect has also been added.

* Add admin UI to Restore/Confirm moderation of notifications

Add the admin UI needed to restore the hidden proposal notifications
when hidden by moderator. The admin can restore them or confirm
the moderation made by moderator, just like proposals.

* Fix restore notifications from admins page

The restore feature was not working properly. When pushed, the button
was removing the notification from the admins panel, but it was not
restoring in the proposal.

I added an `after_restore` function (that I missed in the first PR)
so that the notification is unmarked as moderated.

* Precompile CKEditor plugins in use.

As stated in consuldemocracy#1196, compiling everything related to CKEditor made
compilations slower. However, not compiling any plugins meant Travis had
to compile them while running a test. It often resulted in a test
failing because the time Travis took to compile the plugins the
application uses exceeded Capybara's wait time.

* Always set `Globalize.locale` after `I18n.locale`.

The test "Budget Investments Show milestones" was failing in certain
cases where `Globalize.locale` had been changed in a previous test.

Since having different values in `Globalize.locale` and `I18n.locale`
has proven to be an issue on the test enviroment, this commit also
changes application code in order to avoid similar situations on
production.

See issue consuldemocracy#2718.

* Fix flaky emails spec.

These tests were failing randomly because there is no guarantee
the methods `Budget#email_selected` and `Budget#email_unselected` will
always send the emails in the same order, because `investments.selected`
and `investments.unselected` don't necessarily return the records in the
order they were created.

Ordering by id is certainly not very elegant; however, ordering by
another field like `created_at` is dangerous because the record could be
created at (almost) the exact same time.

Related to issue consuldemocracy#2446 and issue consuldemocracy#2519.

* Check page between AJAX requests in proposals spec.

There was a flaky spec supporting proposals. It's hard to reproduce and
be sure about what was going on, so here is my best guess. Given the
code:

```
within(".proposals-list") { click_link proposal.title }
within("#proposal_#{proposal.id}_votes") { click_link('Support') }
```

The first clicked link generates an AJAX request. Usually, Capybara
would wait for the AJAX request to generate a "Support" link in the
element `#proposal_XX_votes`. However, there's already a
`#proposal_XX_votes` element with a "Support" link on the proposals
index page!

So Capybara doesn't have to wait for the AJAX request to finish before
clicking the "Support" link. From then on, anything can happen.

Another option that works:

```
within(".proposals-list") { click_link proposal.title }
within(".proposal-show #proposal_#{proposal.id}_votes") { click_link('Support') }
```

With this code, the link selector fails on the proposals index page, and
Capybara waits for the AJAX request to finish.

Related to issue consuldemocracy#2558.

* Add issue template

* Removes tablist and tab roles

* Shows flag actions div only if user can hide

* Hides sidebar menus if officer has no shifts assigned

* Removes poll budget creation on specs

* Adds button to officing voters if person decided not to vote

* Adds help gif on legislation processes with texts

* Fixes images size on help pages

* Adss image height to recommendations on homepage

* Fixes lint warnig

* Fixes valuation budget investments ui

* Fixes heading name on budgets index view

* Removes duplicated spec

* Avoid date changes during results tests.

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

* Don't monkey patch ActiveSupport.

It could have side effects (for example, a conflict after upgrading to
Rails 5).

Thanks @aitbw for the suggestion!

* Use dynamic times and dates in factories.

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.

* Avoid date changes during residence tests.

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.

* Deal gracefully with recommendations of hidden proposals

We were seeing an exceptions in the home page when displaying
recommendations. This was due to trying to load tags of hidden proposals

With this commit we are skipping proposals that that have been hidden,
which will hopefully solve this exception

* Deal gracefully with hidden followable in my activity

We were seeing an exception when a user was following a proposal and
that proposal becomes hidden

With this commit we are skipping this proposals and hopefully fixing
this exception

We are also adjusting the count of followed proposals, without
including hidden proposals. It might be cleaner, to remove a `Follow`
when its `Followable` becomes hidden. But then we should probably
activate the `Follow` again if the `Followable` becomes non-hidden…

For now this commit should suffice 😌

* Adds generic annotator help gif

* Adds missing i18n

* updates sprockets

address vulnerability in sprockets CVE-2018-3760.  https://groups.google.com/forum/#!topic/ruby-security-ann/2S9Pwz2i16k

* Add form fields to set the colours and the sections for the banner
Add migrations to store the information in the database

* Apply banner style to the new banners
Banner sections can be saved (one banner can appear in several sections)
If the hex color is changed in the textfield, the color of the color picker changes.

* Specs added to test the change of background and font color with color-picker and text-field

* Make color-pickers smaller

* New and fresh DB schema

* Changes suggested in PR:
* Delete all things related to banner images and styles (in code)
* Add a new test to check that the banner is showing correctly
* Update the specs accordingly to match the changes

Update dev_seed to set a random background_color and a font_color for banners (and remove everything about image and style)
Add a rake task to migrate the banner style to backgroun_color and font_color (so that the banners have the same colors than before)

* Update PR with master

Rebase master branch so that this PR can
be updated with the latest changes.
Conflicts has been solved. dev_seeds
has been also adapted to the new format
(it all separated to be easier to manage).
Some specs has been updated to work with
selenium-webkit.

* Fix banners in user pages

Banners were not been shown in certain pages; now
they are.

Spec to check if the banner is been shown correctly
added. Before it was in admins specs, now it has it's
own spec out of admins folder.

* Fix failing admin/banners scenario

* Correct typo on English locale for banners

* Add max headings label on Group list

* Max headings are updated when editing heading

The max headings label is updated automatically
when updating the heading. It is done via AJAX
and a refresh isn't needed.

* Specs modified to fit the new UI

Now, a page refresh isn't needed to see the
updated information because it is done via AJAX.
The spec has been updated to check that the
message is being correctly updated without the
refresh.

* Stub the Date and Time classes so that the test doesn't depend on the time it is run.

* Removes participate in other polls button

* Adds message in budget investment show if project is winner

* Removes unused i18n keys

* Removes unused i18n keys

* Mitigate recurrent flaky specs for voting comments

* Make Capybara check the page between votes.

As pointed out in PR consuldemocracy#2734:

"After clicking the first link, there's an AJAX request which replaces
the existing `.in-favor a` and `.against a` links with new elements. So
if Capybara tries to click the existing `.against a` link at the same
moment it's being replaced, clicking the link won't generate a new
request".

Making Capybara check the page for new content before clicking the
second link solves the problem, in the same way 4ddc869 solved the same
problem in the comments section.

* Remove focus from specs

* add proposal image on dev_seeds task

* Adds recommended index partial and styles

* Adds recommendations on debates index

* Adds recommendations on proposals index

* Enable 'Recommended debates' setting for users

* Enable 'Recommended proposals' setting for users

* Include tests for recommendations feature

* Add new settings for dev seeds for Debates and Proposals recommendations

* Users can now hide recommendations

* Refactor recommendations specs to improve DRYness

* Wrap recommendations settings checkboxes with label helper to improve A11y

* Improve CSS styles for recommendations partial

* Fixes margin top on recommended index

* Fixes padding bottom on recommended index

* Recommendations for users are enabled by default

* Recommendations are automatically disabled if dismissed by user

* Ensure recommendations are only shown when enabled

* Fix typos for Settings locales (English)

* Move disable recommendations permissions to Abilities::Common model

* Add rake task to enable recommendations for existing users

* Change I18n strings for warning when dismissing recommendations

* Add Rake task to enable recommendations settings

* remove add_images_to argument spaces

Sorry, at work we have a co-worker (@rochgs) that thanks this "spaced" style
because of his visual handicap. Here you are!! ;)

* Regenerate knapsack report

This report is used to distribute specs across Travis’ build matrix

* Fix validation error when creating proposals without user verification

We were getting a validation error when skipping user verification and
creating proposals

This was due to the responsible_name being empty for unverified users

As skipping user verification is a temporary setting used until Census
integration is configured, we can safely skip this validation when this
setting is active

* Fix specs

* Update changelog for release v0.16

* Update version number for consul.json

* Temporary overriding rails-asset url due to ssl problems

* Fixing 016 merging tests
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.

3 participants