We were adding a `visit` in a `before` block but then we started some
tests with another `visit`.
We also destroyed records in the database in between, which increased
the risk of database inconsistency since the process running the browser
had already been started.
Besides, some tests were wrong; they were visiting a page with the
browser, then destroying records in the database, and then checking the
page without reloading the browser. Since we aren't automatically
refreshing the affected areas of the page, obviously the page content
before and after destroying records is exactly the same, and the test
was passing because it's testing content that isn't there in any
situation.
We were updating the database after starting the browser to emulate the
behavior where a user logs in a day before the current request. We can
use `current_sign_in_at` instead and devise will automatically copy that
value to `last_sign_in_at` after users visit a page.
This way we avoid setting up the database after the process runnin the
browser has been started.
Changing the database after the process running the browser has started
is proving to be one of the reasons tests are failing sometimes, so
we're reducing the number of times were that happens. In this case, we
were changing a setting.
We were adding a `visit` in a `before` block but then we started the
search tests with another `visit`.
We also created records in the database in between, which increased the
risk of database inconsistency since the process running the browser had
already been started.
Just like we did in commit 0ec8878db, we remove the useless initial
request in the `before` filter since most tests started by visiting a
different URL.
We also reduce the risk of database inconsistency which comes with
setting up the database after the browser has been started.
Although it wasn't a real security concern because we were only calling
a `find` method based on the user input, it's a good practice to avoid
using constants based on user parameters.
Since we don't use the `find` method anymore but we still need to check
the associated record exists, we're changing the `followable` validation
in the `Follow` model to do exactly that.
The test was hanging sometimes on my machine, probably because we
weren't making sure the request submitting the form had finished before
visiting a new page.
In theory the spec should have been fine from a technical point of view:
since submitting the form generates a regular HTTP request (and not an
AJAX one), Capybara/Selenium/Chromedrive should wait until the request
is finished. But that doesn't seem to be the case 100% of the time;
maybe conditions change depending on previous tests.
On the other hand, from a design point of view, the spec wasn't that
fine. The main purpose of system specs is to test the way users interact
with our application, and users don't click a button and immediately
visit a different page. Instead, most users wait until they receive
feedback of their actions, and then they visit a different page.
Of course some users might visit another page without waiting. What
happens then cannot be predicted (it will depend on which request is
handled first), and so there's no point in writing a test for this case
unless there's a specific concurrency issue we'd like to check.
Even after the previous changes, this test is still failing sometimes
(although now it fails for a different reason). We're doing this change
in order to discard it as the reason why the test is failing.
There seems to be an issue with capybara or chromedriver which results
in `fill_in` sometimes appending to an input rather than overwriting
[1], causing some tests to fail under certain circumstances.
Clearing fields before filling them in solves the issue.
Note we're now getting warnings on all tests using the rack driver. I
haven't found a way to avoid the `clear: :backspace` option in
non-JavaScript tests, so to avoid the annoying warnings we should reduce
the number of tests using the rack driver even more.
[1] See issue 2419 in https://github.com/teamcapybara/capybara/issues
In the Management section when creating an investment we were not passing the
document attributes, so we were never able to associate documents.
Make the nested_documentable spec compatible with the Management section.
In the Management section when creating an investment we were not passing the
images attributes, so we were never able to associate images.
Make the nested_imageable spec compatible with the Management section.
When we assigned `I18n.available_locales = default_locales` in the
`ensure` block, `I18n.locale` was set to `:zh-TW`, which is not one of
the default locales.
In some cases this resulted in tests failing:
```
I18n::InvalidLocale:
:"zh-TW" is not a valid locale
```
We had a test failing several times in GitHub Actions where a user was
still logged in even after logout.
This issue can be reproduced running:
```
rspec spec/system/moderation/proposal_notifications_spec.rb:71 \
spec/system/notifications_spec.rb:126 --order defined
```
One possible cause is a concurrency issue because the process running
the test and the process running the browser both access the same
database connection. Maybe some database operations leak between tests
due to that, particularly if the previous test accessed the database
after starting the browser as well.
A way to avoid this possible cause is setting up the database before
starting the browser with a call to `visit`.
Since the :post_started_at and :post_ended_at fields are of type Date, we check
with Date.current and not with Time.current.
This change has been caused because some test suites were failing
(https://github.com/consul/consul/runs/2170798218?check_suite_focus=true).
The code we had was causing the banners to be available a few hours earlier
or later than they should be depending on the time zone of the application.
Many management actions only make sense if a user has been selected
beforehand.
We updated :check_verified_user method to be able to check actions that need to
have a user selected in order to avoid exceptions.
We need this control as :only_verified_user is not restrictive enough. The reason is
that the :managed_user method used in the :only_verified_user if it does not find a
user it does an initializce (find_or_initialize_by). This causes that when we have
"skip_verification" to true, it returns this non-persisted user as "verified".
These changes affect the actions of Account, Budgets and Proposals Controller
when no user is selected.
We prepare the file to be able to include specs
that do not need to have a logged-in user.
We also took the opportunity to not execute this
line in some specs where it was not necessary.
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.
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.
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.
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.
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.
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.
