The tests adding comments were flaky sometimes. It was not trivial to reproduce the failures, but they seemed to happen often enough to cause some annoyance and I happened to catch a series of them on Travis-CI runs.
Failures:
1) Entries for Challenges Entry exists on a Challenge, user is the owner #comment can comment on an entry
Failure/Error: expect { click_button 'Comment' }.to change { Challenge.first.entries.first.comments.count }.from(0).to(1)
expected `Challenge.first.entries.first.comments.count` to have changed from 0 to 1, but did not change
# ./spec/features/entry_feature_spec.rb:51:in `block (4 levels) in <top (required)>'
After spending quite a while debugging the issue, I finally traced it to the possibility of a race condition on Capybara's click_button and the action completing on the backend.
Note that this race condition only seems to happen when js: true is specified, which is the case with these tests. When that setup is in place, Capybara will use a separate thread for front-end and back-end and it's possible that the back-end thread will not have had the opportunity to update the model before the front-end checks for it. (On the other hand, simply adding a sleep to the back-end was not enough, since it seems the front-end tries to wait for the back-end, but it seems it's not always successful doing so.)
The fix for the issue seems to be calling additional Capybara methods that will wait for the content returned from the back-end to render, which should be enough to ensure completion of the back-end steps. We were already checking for those (with expect(page).to have_css and so on), but that was only happening after the checks on the model (which were failing.)
I fixed this by moving the additional Capybara checks inside the same block, so that the whole request is executed and the output is first checked, before checking the expect {...}.to change of the backend that tests that a comment was indeed created.
Tested: Ran rspec in a loop for ~3 hours after this change and didn't manage to reproduce the problem. On the trunk, without this commit, running it in a loop for ~1 hour was enough to reproduce it.
Tested that modifying some of the expects, both in the block and on the checks for the change in the model were enough to make the tests fail.
(Note: I also tested dropping the js: true from the two tests that add comments and ran rspec in a loop for a few hours. In that case, it seems the issue was also fixed, since it seems Capybara uses a single thread in that case. I believe js: true is not really needed here, so I'll probably send a follow up dropping it. But I thought it would be good to fix the actual issue, in case we end up relying on Javascript in the future and need to keep it enabled.)
These resources were extremely helpful in debugging and fixing this issue:
The tests adding comments were flaky sometimes. It was not trivial to reproduce the failures, but they seemed to happen often enough to cause some annoyance and I happened to catch a series of them on Travis-CI runs.
Here's a recent example: https://travis-ci.org/github/filbranden/vimgolf/jobs/772342117
After spending quite a while debugging the issue, I finally traced it to the possibility of a race condition on Capybara's
click_buttonand the action completing on the backend.Note that this race condition only seems to happen when
js: trueis specified, which is the case with these tests. When that setup is in place, Capybara will use a separate thread for front-end and back-end and it's possible that the back-end thread will not have had the opportunity to update the model before the front-end checks for it. (On the other hand, simply adding asleepto the back-end was not enough, since it seems the front-end tries to wait for the back-end, but it seems it's not always successful doing so.)The fix for the issue seems to be calling additional Capybara methods that will wait for the content returned from the back-end to render, which should be enough to ensure completion of the back-end steps. We were already checking for those (with
expect(page).to have_cssand so on), but that was only happening after the checks on the model (which were failing.)I fixed this by moving the additional Capybara checks inside the same block, so that the whole request is executed and the output is first checked, before checking the
expect {...}.to changeof the backend that tests that a comment was indeed created.Tested: Ran
rspecin a loop for ~3 hours after this change and didn't manage to reproduce the problem. On the trunk, without this commit, running it in a loop for ~1 hour was enough to reproduce it.Tested that modifying some of the
expects, both in the block and on the checks for thechangein the model were enough to make the tests fail.(Note: I also tested dropping the
js: truefrom the two tests that add comments and ranrspecin a loop for a few hours. In that case, it seems the issue was also fixed, since it seems Capybara uses a single thread in that case. I believejs: trueis not really needed here, so I'll probably send a follow up dropping it. But I thought it would be good to fix the actual issue, in case we end up relying on Javascript in the future and need to keep it enabled.)These resources were extremely helpful in debugging and fixing this issue: