Fix flaky specs: Votes Debates and Voting comments Update #2734
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
javierv
changed the title
|
Hmmm @javierv we try to use a as little javascript as possible, it's not very pretty and it can have some unexpected side effects. We are trying to solve just a flaky spec in this PR. I don't see the need to add javascript to change, the always delicate, voting system. +1 to using Capybara to check for the correct links being present |
|
@voodoorai2000 👍 I'll make the changes before Monday. After opening this pull request, I realized #2448 solved a similar issue affecting different tests. However, it wasn't merged because switching to chromedriver apparently made the flakies disappear (although unfortunately it only made them happen less frequently). @voodoorai2000 @aitbw So I'd suggest I change this pull request so it uses the same solution #2448 does (making Capybara check the page), and then we merge both pull requests. What do you think? |
|
+1 @javierv 👌 |
|
We definitely could take a look at it 🤔 can you confirm the flakies #2448 aimed to solve are fixed over AyuntamientoMadrid#consul, @javierv ? Thank you so much for your time! 🙏 |
|
Mine too 😌 I don’t quite see how it solves the flackiness, apart from possible session data. In any case I would expect Capybara to make a clean session slate on every test But if it fails without the fix... and it passes with the fix 👍🏻👍🏻👍🏻 |
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.
javierv
force-pushed
the
2713-fix_flaky_update_votes_spec
branch
from
515bb70
to
32504b8
Compare
javierv
changed the title
javierv
changed the title
|
Sorry I forgot to clarify #2448 did two things:
AFAIK the first change didn't affect these specs; however, the second did. There was a small typo in that PR, though, since after voting in favor it was checking for "0" instead of "1" as the content of votes in favor. So now this PR has two commits, rebased against current master: one keeping the changes in #2448 which solved the problem and fixing the "1" typo (all credit to @aitbw), and another one implemeting the same solution when voting a debate. On my machine all these specs still fail on master sometimes. After applying these changes, none of them do. |
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
3 tasks
2 tasks
javierm
referenced
this pull request
in javierm/consul
Oct 23, 2018
As pointed out in PR consul#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. This commit solves issues afecting both Madrid's fork and the original CONSUL repo.
javierm
referenced
this pull request
in javierm/consul
Oct 23, 2018
As pointed out in PR consul#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. This commit solves issues affecting Madrid's fork but not the original CONSUL repo.
javierm
referenced
this pull request
in javierm/consul
Oct 29, 2018
As pointed out in PR consul#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. This commit solves issues afecting both Madrid's fork and the original CONSUL repo.
decabeza
referenced
this pull request
Nov 5, 2018
As pointed out in PR consul#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. This commit solves issues afecting both Madrid's fork and the original CONSUL repo.
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
voting commentsflaky specs #2448Objectives
Fix the flaky specs that appeared in
spec/features/votes_spec.rb:82("Votes Debates Single debate Update"),spec/features/comments/polls_spec.rb:495("Commenting polls Voting comments Update"),spec/features/comments/topics_spec.rb:525("Commenting topics from proposals Voting comments Update"),spec/features/comments/topics_spec.rb:525("Commenting topics from budget investments Voting comments Update"),spec/features/comments/debates_spec.rb:486("Commenting debates Voting comments Update"),spec/features/comments/budget_investments_spec.rb:481("Commenting Budget::Investments Voting comments Update") andspec/features/comments/legislation_annotations_spec.rb:562("Commenting legislation questions Voting comments Update").Explain why the test is flaky, or under which conditions/scenario it fails randomly
Here's the relevant part of the spec:
After clicking the first link, there's an AJAX request which replaces the existing
.in-favor aand.against alinks with new elements. So if Capybara tries to click the existing.against alink at the same moment it's being replaced, clicking the link won't generate a new request, similar to what happened in #2696.Explain why your PR fixes it
Making Capybara check the page for new content before clicking the second link solves the problem.
Original solution (obsolete)
Another option would be to update the content without replacing the existing links, so it's always available for Capybara to click. That way would also solve a small issue affecting keyboard users (note the way the focus disappears after clicking an icon, and how it goes to the "like" icon when pressing "tab"):
Since the links are regenerated after being clicked, the focus goes back to the element being replaced, and that leads to awkward behaviour like clicking "dislike", pressing "tab" and the focus going back to "like".
By changing the content without regenerating the links, the links wouldn't lose focus after being clicked and the "tab" key behaves as expected: