As mentioned in commits like a586ba806, a7664ad81, 006128da5, b41fbfa52
and c480cdd91, accessing the database after starting the browser with
the `visit` method sometimes results in database corruption and failing
tests on our CI due to the process running the test accessing the
database after the process running the browser has started.
In this case, we're avoiding the usage of `user.subscriptions_token` and
`Comment.last`. In the future, we should probably simplify these tests
by moving most of the checks to a mailer test.
As mentioned in commits like a586ba806, a7664ad81, 006128da5, b41fbfa52
and c480cdd91, accessing the database after starting the browser with
the `visit` method sometimes results in database corruption and failing
tests on our CI due to the process running the test accessing the
database after the process running the browser has started.
Note that, in this case, in order to make the tests more readable, we're
adding a bit of duplication. We should probably simplify these tests by
moving most of the checks to mailer tests and then we could remove the
duplication. The alternative would be to make the
`create_direct_message` method way more complex than it is right now.
These tests don't use the browser to send emails since commit e21588ec1.
However, note that this commit actually actually decreased our test
coverage somehow; since then, we're no longer testing whether we send an
email to the author after clicking the "Publish comment" button. We
might need to add a test for this in the `spec/system/comments_spec.rb`
file... but that's a story for another time.
Note we need to stub the `deliver_later` method; otherwise,
`open_last_email` would raise an exception since no email would have
been delivered.
We're moving them now because these tests use the `open_last_email`
method, and we're looking for places where using this method might
result in a flaky test when used inside a system test.
These tests were testing model methods, but were inside files for system
tests.
We're moving them now because these tests use the `open_last_email`
method, and we're looking for places where using this method might
result in a flaky test when used inside a system test.
Note we're excluding a few files:
* Configuration files that weren't generated by us
* Migration files that weren't generated by us
* The Gemfile, since it includes an important comment that must be on
the same line as the gem declaration
* The Budget::Stats class, since the heading statistics are a mess and
having shorter lines would require a lot of refactoring
This test has been failing many times because it checks the database
after starting the process running the browser.
We're disabling JavaScript like we do in every other test using the
`create_proposal_notification` method. This way, Capybara will use the
rack driver and there'll be no risk of errors that currently might take
place if both the process running the browser and the process running the
test access the database.
Currently the translation:
"Notify me by email when someone comments on my proposals or debates"
It only refers to proposals and debates, but actually it also refers to budget
investments, topics and polls.
We modified the link that previously redirected us to the "My content"
page to redirect us to the new page for managing subscriptions.
We also adapted the existing generic text by adding a description of
the related notification.
We modified the link that previously redirected us to the "My content"
page to redirect us to the new page for managing subscriptions.
We also adapted the existing generic text by adding a description of
the related notification.
Sometimes tests were hanging indefinitely. Debugging shows that in some
cases it's due to submitting a form before the AJAX request to get
proposals, debates or investments suggestions is finished, since having
an AJAX and a non-AJAX request at the same time when running the test
sometimes leads to unexpected results.
In our case, we were having many timeouts in Github Actions in the
branches where we use both ActiveStorage and Paperclip to store files
(based on pull request 4598). I can reproduce it in those branches
running the following test ("Should show new image after successful
creation with one uploaded file"), although only when my laptop isn't
plugged (!!):
```
rspec './spec/system/proposals_spec.rb[1:33:1:14]'
```
Since we didn't have a proper way to know the AJAX request had finished,
we're adding a `suggest-success` class to the element showing the
suggestions when that happens. Then in the tests we can look for that
class after filling in the title of a proposal, debate or investments.
Just for clarity's sake, we're also adding the `suggest-loading` class
when the suggestions are loading.
In order not to have expectations everywhere about the suggestions,
we're extracting methods to fill in those titles in the tests. Note we
aren't using these methods in the "edit" actions (suggestions are not
showing when editing) or in tests with the `no_js` tag (since
suggestions only work with JavaScript).
While we use Pronto to detect offenses in the lines changed in our pull
request, sometimes our changes introduce offenses in other lines, and we
don't detect them.
In commit 0488b3735, we removed the only usage of the `heading` method
in a test, which caused a `RSpec/LetSetup` offense.
In commit 287c48873, we changed some lines from `fill_in` to
`fill_in_ckeditor`. Some of these lines were aligned with the following
ones, which after that change had extra spacing for no reason.
Finally, in commit 8d38ed58c we added a line before two lines which had
their equals signs aligned. Since, after adding this line, the block was
no longer aligned, there was no reason for the extra space in one of the
lines.
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.
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.
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.
We're improving the readability of the ones we're about to modify.
Using human texts makes tests easier to read and guarantees we're
testing from the user's point of view. For instance, if we write
`fill_in banner_target_url`, the test will pass even if the field has no
label associated to it. However, `fill_in "Link"` makes sure there's a
field with an associated label.
So tests won't fail when an institution changes the default organization
name.
The tests are also easier to understand now, since it's more obvious
where the "CONSUL" text is coming from.
We were repeating the same code over and over (with a few variants) to
setup tests which require an administrator. We can use a tag and
simplify the code.
