Fix ckeditor specs failures #15
Merged
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
microweb10
force-pushed
the
fix_ckeditor_specs_failures
branch
from
d9d4c24
to
0acef1c
Compare
In this case the confirmation dialog isn't really necessary since the action to enable/disable the setting can easily be undone. Furthermore, these tests were failing with Chrome 83, probably because we use `confirm_dialog` and then we use `visit` without checking the page in between. In theory we shouldn't need to check the page in between because the request generated by `confirm_dialog` is a synchronous one and so `visit` isn't executed after the previous request has finished, but apparently this behavior has changed in Chrome 83. We could add an expectation before executing the `visit` method, but that wouldn't improve the usability of the application.
This gem will automatically install chromedriver based on the installed version of Chrome/Chromium.
The latest stable version is causing problems on some machines, hanging forever in tests involving frames. So we're installing an old version which works with the latest Chrome. Note this means we're using an unsupported version. Officially, only the latest chromedriver supports the latest Chrome. We're using 2.38 instead of a more recent one (like 2.40) because it's the one we specified in our Dockerfile. See also: https://bugs.chromium.org/p/chromedriver/issues/detail?id=3361
With chromedriver >= 80, the tests are freezing sometimes, particularly when the same editor is loaded again. We don't know whether it's a CKEditor issue or a chromedriver issue. In the past we've had some errors related to CKEditor trying to load the same instance twice and we aren't sure they have been fixed since we could never reproduce them. It could be a coincidence, though. If we modify the views so the only content of the `<body>` tag is a textarea with the `html-area` class, chromedriver freezes even if we only access the page once. So maybe we're only detecting the problem on the second visit because the second request is faster than the first one. Since chromedriver no longer hangs after this change, we don't have to force any chromedriver version anymore.
We've simplified the way CKEditor is handled in tests; probably due to that, we don't need this method anymore.
It looks like sometimes, particularly when the first thing we do after loading a page is filling the CKEditor fields and submitting the form, CKEditor doesn't have enough time to format the text, and so it's sent as plain text instead of HTML. This behaviour can be reproduced on my local machine after upgrading to Rails 5.1, with the test "Admin Active polls Add" failing 100% of the time. Checking CKEditor has been filled in correctly solves the issue.
Some specs involving CKEditor were failing sometimes in the Rails 5.1 branch. The reason why these specs pass with Rails 5.0 but fail with Rails 5.1 are unknown. On my machine the tests pass when precompiling the assets, which makes me think it's related to the way Rails handles them, but it might have nothing to do with it. The only (apparently) 100% reliable solution I've found is to wait for CKEditor to load before trying to fill it in. After running the tests on my machine hundreds of time, I didn't get a single failure.
I incorrectly used "text" as variable name in commit 2cdc6a1. In similar places, we use `label`. We also use named parameters when only `with:` is provided.
After upgrading to chromedriver 80, tests checking CKEditor's content were causing chromedriver to hang. That's why we were configuring webdrivers to use an older chromedriver. Version 80 of chromedriver introduced several issues regarding frames. Debugging shows in this case chromedriver froze when we used `setData` and then `within_frame`. Since adding a `sleep` call made it work, we think `within_frame` was being executed before `setData` had finished. The fact that `setData` causes the browser to enter the frame having CKEditor is probably the reason. Even though the `setData` method provides a callback when it's finished, configuring it so the rest of the Ruby code isn't executed until that happens leads to complex code. Using Capybara's `set` to fill in the editor is IMHO a bit easier to understand. After this change, since we're using a method provided by Capybara instead of executing asynchronous JavaScript code, we don't have to check CKEditor has been filled anymore. The "Admin Active polls add" test, which failed on my machine without that check, now passes.
If we don't use the `exact` option, tests will pass even if filling in CKEditor adds the content twice or adds the new content to the existing content, which has actually happened and has gone mostly unnoticed while testing several ways to fill in CKEditor with Capybara (particularly, when using Capybara's `send_keys` method). The problem was detected by just one test, which checked the original content wasn't present anymore after updating a record.
microweb10
force-pushed
the
fix_ckeditor_specs_failures
branch
from
0acef1c
to
0c07723
Compare
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.
Objectives
Fix failures in specs using CKEditor
Notes
Commit list:
consuldemocracy@5bf968d
consuldemocracy@59b625a
consuldemocracy@2c4acb0
consuldemocracy@72c2b87
consuldemocracy@a0ea1f6
consuldemocracy@2cdc6a1
consuldemocracy@5b97b20
consuldemocracy@a103b53
consuldemocracy@8408bfd
consuldemocracy@4d65507