Reduce test database access after visiting a page #4475
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}
javierm
force-pushed
the
browser_tests_database
branch
3 times, most recently
from
003b921 to
112aba5
Compare
Just like we mentioned in commit 5f0c422, the filter "Mark as viewed" doesn't work properly, so here's a case where the test would fail with JavaScript not because the test is wrong, but due to a bug. The test was passing simplify because there was a typo in the CSS selector, which was supposed to select an element by ID but didn't have the "#" prefix. For now we're keeping the test as it was, but eventually we'll have to fix the bug.
We were checking for a CSS ID selector but forgot to add the "#".
We were checking what happens in the database, but what's important is what users experience after hiding content or blocking authors. Besides, accessing the database in tests after the browser has started might lead to database inconsistencies.
javierm
force-pushed
the
browser_tests_database
branch
11 times, most recently
from
72be397 to
3b024bc
Compare
These tests check what happens from the user's point of view. For instance, we check that after disabling recommendations, they are not shown. What happens in the database is not related to the user experience. Furthermore, checking the database after the browser has started is proving to be a major source for inconsistent data in specs.
Using "reload" might cause issues if the process running the browser has already been started, and it isn't necessary in these cases.
We check the changes have been saved and we check recommendations have been disabled after visiting the debates and proposals pages. The latter helps us avoid accessing the database after the process running the browser has been started.
We were checking the database, but users don't care about what's inside the database; they care about what happens when they visit the page of a record they've just restored. This way we also avoid data inconsistency due to the process running the test accessing the database after the process running the browser has started.
We already test the navigation in the valuation spec file.
System tests are about user experience, so instead of checking the slug has been updated in the database, we check whether the page can be accessed using the slug. Note the budget group test is a bit different because the name of the group isn't present in the budget group page.
This scenario is already tested in the management users spec: ""Delete a level 2 user account from document verification page".
What users care about isn't the database; they care about that reason being displayed when administrators check the reason. This way we also avoid accessing the database after the process running the browser has been started.
We take the comment as a parameter instead of the user, since usually people reply to comments and not to users. We also remove one database query after the browser has started, since we can use `debate_path(debate)`. It's also more clear why we're using `debate_path` in the test; before these changes, we had to enter the `reply_to` method to realize that we were replying on a debate.
Checking the database with methods like Activity.last does not test that the record is present where it should be (first record of the table in this case). In these tests there's only one record, though, so the order doesn't matter that match. However, calling methods like Activity.last generates a database query after the process running the browser has been started, and this might lead to inconsistent data.
This way we make sure the label is correctly associated with a field.
Users don't care about database content; they care about what they see on the screen. Writing tests this way we also avoid potencial database inconsistencies due to accessing the database after starting the browser.
This way we avoid modifying the database in the middle of a system test (after we've started the browser), which can lead to database inconsistencies. In the case of the reclassification specs we're simply removing part of the test because that part is already tested by other specs.
It's strange to create records without assigning them to a variable and then query the database to fetch the very same records. Assigning them to a variable makes the tests easier to understand. Besides, this way we avoid querying the database after the browser has started.
When we create a record like a debate or an event and we check the page content, we want to make sure that today's date is present, since it's the date where the record is supposed to have been created. This way we avoid querying the database after the browser has been started.
We can assign query results to variables and so we avoid querying the database after starting the browser.
In general, we shold check the contents of the page instead of the current path, since the contents of the page are what users experience. In one test, the only reason we check the current path additionally to the contents of the page is to make sure we're still in the management section. Checking just that we avoid querying the database after starting the browser.
We want to make sure the request is finished after clicking a button and before visiting a different page, so we need to check the page has changed. Usually this shouldn't be a problem because most of our forms are sent with regular HTTP requests instead of AJAX ones, so the `visit` method wouldn't be called before the request is finished. However, we're experiencing problems with certain version of Chromedriver, and in general it's a good practice because we might send forms using AJAX/Turbolinks in the future.
We were checking content which was already present/absent before making a certain request, so the expectations were not checking the request had already finished. Our intention here is to check the page contents after the request has finished.
So we check the message with the verification link is present and administrators see the organization in the admin section.
It's true that previously we didn't display the tag cloud on all phases and so we added a test checking we did on all phases. However, doing so makes tests really slow and prone to database inconsistencies because the alter the database after the process running the browser has started. So now we're using a random phase in these tests to solve this issue. We're also removing the `login_as(admin) if budget.drafting?` line because we removed the drafting phase in commit 28caabe.
The link to edit the process is already present before clicking the "All" link, which meant the test failed sometimes because Capybara might try to click on the "Edit" link at the same time the page is changing due to the click on the "All" link". Due to this issue, this test has failed at least one in our CI [1]. [1] https://github.com/consul/consul/runs/2324773853
The controller provided by the `devise-security` gem which tests password is expired does not execute the `before_action` we have in our application controller. That means it doesn't set the current locale. We were having issues in the tests checking this behavior if the previous test had set the current locale to a different one. This meant the process running the browser had one locale while the process running the test had a different one, which resulted in a page in English (as expected), only the flash message notifying users their password expired was in a different language. To reproduce this behavior, run: ``` rspec './spec/system/welcome_spec.rb[1:1:2:2:1]' spec/system/users_auth_spec.rb:623 --order defined ``` I'm not sure whether this is a bug or it's a problem with the tests. In theory it might be possible to reproduce a similar behavior in production due to what we mention about the controller not executing the `set_current_locale` method. But I haven't been able to reproduce the situation, particularly since the password expiration seems to be checked exclusively at login time (that is, if you stay logged in for 10 years, your password doesn't seem to expire). So for now I'm just making the tests pass by using the login form instead of using `login_as`.
We only need to define one `in_browser`, which is the one opening the session as an administrator. This change is done to simplify the code, although there's a small chance it might also make the test stop failing in our CI. Sometimes in our CI the first `visit` in the `in_browser(:admin)` block fails for unknown reasons, rendering a blank page.
javierm
force-pushed
the
browser_tests_database
branch
from
3b024bc to
3345dd6
Compare
taitus
approved these changes
Apr 16, 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
Notes
We haven't solved all cases affected by this issue. A quick search for terms like
.last,.first,.count,.destroy,.updateor.find_in the system tests shows us we'll still have to revisit this issue in the future.