[WIP] Fix flaky spec: Budget Investments - Balloting Phase - Confirm

[microweb10]

References

Issue #3035

Explain why the test is flaky

Apparently Capybara can execute a piece of JS before other execution of JS has completely finished.

Explain why your PR fixes it

Using execute_script method will add click() function to the bottom of JS call stack and the click will be executed after other JS functions are finished except for the ones using async callbacks.

Notes

I've executed the test 150 times after the change and passed 100% of the times

for run in {1..150}
do
  bin/rspec ./spec/features/budgets/investments_spec.rb:1450
done

[javierm]

Hi @microweb10. Thank you for this pull request (and bug report)!

Unfortunately I couldn't reproduce the issue locally after running the test 60 times on the master branch, so most of what I'm saying are guesses based on similar flaky tests we've had in the past.

It looks like the view in app/views/budgets/ballot/lines/create.js.erb re-renders the ballot the user has selected and then re-renders each ballot.

So then, if capybara clicks the .add a link in the second ballot at the same time it's being re-rendered, the click is ignored.

Since we try to describe feature specs as close to what users do as possible, we try not to execute JavaScript manually during a test. So, if possible, try to keep the original line find('.add a').click (or maybe change it to click_link "Vote", which does exactly the same but it's closer to what users see).

My suggestion would be either to add an extra line after expect(page).to have_content "Remove" checking all ballots have been re-rendered or, if that's not possible, to remove the render 'refresh_ballots' part in app/views/budgets/ballot/lines/create.js.erb. I'm not familiar with this code so I don't know whether it makes sense.

What do you think?

[microweb10]

@javierm it's curious that you cannot reproduce the failure. For me, it was not the first time I saw the same spec failing.

Anyway, I agree and will keep investigating. Hopefully, I will find a good solution 😅

Thanks for the feedback!

[javierm]

Yes, sometimes these flaky tests are quite a pain to debug. I remember there was one which used to fail on my machine about once every five times (I think it was the one fixed in #2995), but after we added some unrelated code which made requests a bit slower, I couldn't reproduce it on my machine unless I deleted the mentioned code 😄, even though it still failed on Travis builds once in a while.

So it looks like depending on our machines we will be able to reproduce different flaky specs 🙄.

[microweb10]

@javierm I don't have very good news 😕

I've tried:

• To check all the ballots are re-rendered after clicking the link.
• To remove the part that refreshes all ballots.
• To remove the part that re-renders the ballot the user has selected and leave the part that refreshes all ballots.

Actually I think the last option is the one we should use. I don't see the point of rendering twice the ballot the user has selected. render 'refresh_ballots' will render all of them 😄.

But I'm still able to reproduce the flaky spec 😓.
I'm running out of ideas 😭.

[javierm]

Interesting! How did you check all ballots had been re-rendered?

A tip on debugging; you add the following code to spec/spec_helper.rb:

  config.after(:each, type: :feature) do |example|
    if example.exception
      save_and_open_page
      save_and_open_screenshot
    end
  end

If the screenshots are too small, you can change the size by changing a line in spec/rails_helper.rb:

    chromeOptions: { args: %w(headless no-sandbox window-size=1200,1600) }

Feel free to share the screenshots with the GitHub community 😉.

[microweb10]

To check if all ballots are re-rendered I added the following code at the end of the method add_to_ballot(budget_investment)

    Budget::Investment.selected.by_heading(heading.id).each do |budget_investment|
      within("#budget_investment_#{budget_investment.id}") { find('.ballot a') }
    end

Apparently that code works fine cause the method find doesn't raise any exception, but the flaky spec fails at the same point, on the line expect(page).to have_content "Remove"

Yeah, I knew about the save_and_open_page and save_and_open_screenshot methods (thanks anyway ☺️), but actually it doesn't give me any useful information, the ballot appears like the "Vote" button hasn't been clicked (while apparently has been found and clicked correclty as no other errors are raised 😐)

I attached an example just tested now:

• This is the error

Failures:

  1) Budget Investments Balloting Phase Confirm
     Failure/Error: expect(page).to have_content "Remove"
       expected to find text "Remove" in "Budget Investment 2 title 2018-11-15 • Manuela3 • Global Heading Spend money on this €10 Vote"

• This is the screenshot

[javierm]

Correct me if I'm wrong 🙏. I think the code:

    Budget::Investment.selected.by_heading(heading.id).each do |budget_investment|
      within("#budget_investment_#{budget_investment.id}") { find('.ballot a') }
    end

Doesn't really check all ballots have been re-rendered, because the .ballot a selector is also there before re-rendering.

Thank you for sharing the screenshot! After seeing it, I'm more confident the problem is related to the find('.add a').click code being executed at the same time ballots are being re-rendered. I wonder if there's a way to make sure ballots have been re-rendered before clicking the .add a link 🤔.

[microweb10]

I'm afraid you are more than right, it doesn't really check it 😅

But, there should be a way, I will keep thinking about it.

The funny thing is that I also tried to remove the part that re-render all ballots and only re-render the one selected, but I was still getting failures.

Thanks for your help! I will keep investigating 😊

[javierm]

The funny thing is that I also tried to remove the part that re-render all ballots and only re-render the one selected, but I was still getting failures.

That's what really puzzles me about this issue 🤔.

[microweb10]

@javierm Yeah, me too. But I have tested it several times and still get the same flaky spec.

After some more investigation I found out that the part that re-renders all the ballots was kind of needed. If removed, specs like Ballots Permissions Insufficient funds (added after create) in spec/features/budgets/ballots_spec.rb will fail.

The part that is not really needed is the one that re-renders the ballot that has been selected, as all ballots will be re-rendered after. So I removed that part.

In a effort to make sure all ballots are re-rendered before clicking the link "Vote" I tried to add an HTML tag to the page at the beginning of the rendering in app/views/budgets/ballot/lines/create.js.erb and removing it after the ballot has been rendered (or at least that I thought) like this:

$("#budget-investments").append("<span id='renderingBallots' />");

$("#progress_bar").html('<%= j render("/budgets/ballot/progress_bar", ballot: @ballot) %>');
$("#sidebar").html('<%= j render("/budgets/investments/sidebar") %>');

$("#<%= dom_id(@investment) %>_ballot").html('<%= j render("/budgets/investments/ballot",
                                                           investment: @investment,
                                                           investment_ids: @investment_ids,
                                                           ballot: @ballot) %>');


$("#renderingBallots").remove();

And making sure there wasn't any span tag with that id before clicking the link.

expect(page).to_not have_xpath "//span[@id='renderingBallots']", visible: false

But I didn't have any luck. I was still getting the same failure.
Also tried the code that appends the tag in the file app/views/budgets/ballot/lines/_refresh_ballots.js.erb where all ballots are re-rendered, but got the same result 😓. Tried also with strategy of adding and removing classes to existings tags, but same result.

Looks like something is still executing when Capybara clicks the link, cause I tried to print some outputs for debugging in the controller. And the method create in app/controllers/budgets/ballot/lines_controller.rb apperars to not be triggered at all 🤔, although the link href property looked fine 😩

So, the only idea I had was to make use of the page.execute_script to "make sure all ballots are re-rendered". But for that I had to use a method against the link that actually does something, otherwise I was still getting the failure. After trying a lot of them I found one that worked for me focus().
It doesn't really look a good/elegant solution for me 😒 (just only the one that works)

Well, of course we could reload the page before adding investments to ballot, but it doesn't look like the right thing to do as well 😞

Let me know if you have any idea, cause I'm running out of them 😅

[javierm]

The part that is not really needed is the one that re-renders the ballot that has been selected, as all ballots will be re-rendered after. So I removed that part.

That sounds good 👍.

And the method create in app/controllers/budgets/ballot/lines_controller.rb apperars to not be triggered at all

That's what I expected, since that's what always happens in similar flaky specs where links are replaced at the same time capybara tries to click them: no request is sent to the server.

I'll let you know if I've got more ideas about how to fix the spec.

[javierm]

Closing in favor of #3113.