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.
