Use a driver supporting JavaScript in system tests #4454
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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -257,7 +257,7 @@ | |||
| expect(page).to have_content comment.body | |||
|
|
|||
| expect(page).to have_link "Cleaner city", | |||
| href: admin_budget_budget_investment_url(investment.budget, investment, anchor: "comments") | |||
| href: admin_budget_budget_investment_url(investment.budget, investment, anchor: "comments", host: app_host) | |||
There was a problem hiding this comment.
Layout/LineLength: Line is too long. [115/110] (https://rubystyle.guide#max-line-length)
javierm
force-pushed
the
browser_tests
branch
2 times, most recently
from
19d075c
to
97f218a
Compare
javierm
force-pushed
the
stats_go_back
branch
2 times, most recently
from
6cdec3c
to
149180b
Compare
javierm
force-pushed
the
browser_tests
branch
3 times, most recently
from
c7a8e78
to
6a6de27
Compare
javierm
force-pushed
the
browser_tests
branch
9 times, most recently
from
e275494
to
10495aa
Compare
It was strange to specify the unfeasibility reason without marking the investment as unfeasible.
Before clicking the "Edit phase" link, there's already a "Name" field present in the page (the name of the budget). With the rack driver, there's no problem since the `fill_in` action waits until the page is loaded. However, the test will be a flaky spec if we use a driver supporting JavaScript, since clicking the "Edit phase" link will cause an AJAX request and the `fill_in` action might be executed before the AJAX request is finished.
This way we reproduce the user experience in the tests, and we can make sure modal dialogs open when we expect it.
We were filling in textareas, so we were only testing how the application behaves for about 1%-2% of our users.
The markdown editor only works with JavaScript, so in order to test it we need a JavaScript test.
IMHO opening new windows is a usability issue which has been known for twenty years since it takes control away from the user and breaks the "back button", but for now we're keeping the same behavior as we already had, while slightly increasing the complexity of the tests (which is a good indicator of a usability issue).
So now we feel in the tests the same usability problems users feel when they want to click a link and the flash message is in their way.
Having all the text show when hovering over a cell is an accessibility/usability issue that we now experience in the tests as well.
We're implicitly checking the link is listed on the menu by clicking it in other tests.
This menu requires JavaScript to open/close subnavigation menus, so we're now testing the way users with a browser supporting JavaScript (98%-99% of the users) deal with the menu.
This way we click on the "menu" link first before clicking on any sections, just like real users do.
So what we write in the tests is close to what users experience.
Content like lowercase letters with `text-transform: uppercase` or spaces after elements with `display: block` or "You're on page:" are not seen that way by users with a browser supporting CSS. So we're testing what most users actually experience.
The test is failing on my machine sometimes. I thought it was because `open_last_email` didn't wait fot the request generated by clicking the "Registrarse" button to finish. Even if that might be the case, adding an extra expectation showed the test was really failing because invisible captcha reported the form was filled in too fast. I wonder whether invisible captcha is working properly or there are times where it doesn't start its timer when it should. Since this might be a bug in the application, I'm not changing the test just to make it pass.
JavaScript is used by about 98% of web users, so by testing without it enabled, we're only testing that the application works for a very reduced number of users. We proceeded this way in the past because CONSUL started using Rails 4.2 and truncating the database between JavaScript tests with database cleaner, which made these tests terribly slow. When we upgraded to Rails 5.1 and introduced system tests, we started using database transactions in JavaScript tests, making these tests much faster. So now we can use JavaScript tests everywhere without critically slowing down our test suite.
This offense was introduced in commit 28caabe.
We've got quite a messy hack to sign in managers: they need to visit a specific URL (management root path). That means tests signing in managers start the browser to sign them in, which might cause issues if we setup the database after that.
GitHub Actions is failing to finish sometimes. Usually that happens due to concurrency issues when the process running the test accesses the database after the process running the browser has started. Since these files were the ones being tested the times we had this issue, these are the ones we are fixing right now, although there are probably other places where we might have this issue in the future.
Sometimes a test gets stuck and and we have to wait until it times out in order to check which files were being tested at the time. The default timeout is six hours. I'm reducing it to one hour which should still be plenty of time even on repositories with no knapsack token.
That way we make sure the request is finished before the test finishes. We were getting a failure in GitHub Actions because an unrelated test executed after this one had its locale set to Spanish (just for one test, though), and one possible explanation could be that a previous request which changed I18n.locale was still being executed.
This was referenced Apr 13, 2021
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
Objectives
:jstag on every test we writeNotes
In the past, using a headless browser in tests made the test suite much slower because we had to use database cleaner to truncate the database after each test. With system tests, however, we use database transactions to make sure data is removed between tests, and so the speed difference isn't that big.