We were very inconsistent regarding these rules.
Personally I prefer no empty lines around blocks, clases, etc... as
recommended by the Ruby style guide [1], and they're the default values
in rubocop, so those are the settings I'm applying.
The exception is the `private` access modifier, since we were leaving
empty lines around it most of the time. That's the default rubocop rule
as well. Personally I don't have a strong preference about this one.
[1] https://rubystyle.guide/#empty-lines-around-bodies
Having exceptions is better than having silent bugs.
There are a few methods I've kept the same way they were.
The `RelatedContentScore#score_with_opposite` method is a bit peculiar:
it creates scores for both itself and the opposite related content,
which means the opposite related content will try to create the same
scores as well.
We've already got a test to check `Budget::Ballot#add_investment` when
creating a line fails ("Edge case voting a non-elegible investment").
Finally, the method `User#send_oauth_confirmation_instructions` doesn't
update the record when the email address isn't already present, leading
to the test "Try to register with the email of an already existing user,
when an unconfirmed email was provided by oauth" fo fail if we raise an
exception for an invalid user. That's because updating a user's email
doesn't update the database automatically, but instead a confirmation
email is sent.
There are also a few false positives for classes which don't have bang
methods (like the GraphQL classes) or destroying attachments.
For these reasons, I'm adding the rule with a "Refactor" severity,
meaning it's a rule we can break if necessary.
We couldn't declare them inside the block because they would be
considered local variables and its value would be lost when the block
was finished. So we were using instance variables instead.
However, with instance variables we don't get any warnings when we
misspell their names. We can avoid them by declaring the local variables
before the block starts.
These variables can be considered a block, and so removing them doesn't
make the test much harder to undestand.
Sometimes these variables formed the setup, sometimes they formed an
isolated part of the setup, and sometimes they were the part of the test
that made the test different from other tests.
We were creating records with a title we manually set, so to be
consistent with the rest of the code, in the test we check the title is
present using a string literal.
This way we can also remove useless assignments while keeping the code
vertically aligned.
This way we write the tests from the user's point of view: users can see
(for example) a proposal with the title "Make everything awesome", but
they don't see a proposal with a certain ID.
There are probably dozens, if not hundreds, of places where we could
write tests this way. However, it's very hard to filter which ones are
safe to edit, since not many of them have an HTML class we can use in
the tests, and adding a class might generate conflicts with CSS styles.
So, for now, I'm only changing the ones allowing us to cleanly remove
useless assignements while maintaining the code vertically aligned.
The group is automatically assigned when we assign the heading. The
budget isn't needed either, except for a special case related to the
reason to be rejected.
These settings are enabled by default.
It could be argued explicitely enabling the features makes tests more
consistent, because they'll work if we change the default setting. It
could also be argued that it makes tests more expressive because it
makes the reader realize certain things will only work if a setting is
enabled.
However, we were only doing so in a few tests. The truth is, thousands
of our tests depend on certain features being enabled. So IMHO we should
be consistent and either set them on every test, or not at all. I'm
choosing the latter option for simplicity.
Settings are stored in the database, and so any changes to the settings
done during the tests are automatically rolled back between one test and
the next one.
There were also a few places where we weren't using an `after` block but
changing the setting at the end of the test.
After adding investment translatable fields to forms, they will be
generated with nested translations names and ids so we can no longer
use standard id locator.
Using input label text to fill in fields works with both types of forms.
The `type: :feature` is automatically detected by RSpec because these
tests are inside the `spec/features` folder. Using `feature` re-adds a
`type: :feature` to these files, which will result in a conflict when we
upgrade to Rails 5.1's system tests.
Because of this change, we also need to change `background` to `before`
or else these tests will fail.
There was a typo: `new_order = eq(all(` instead of `new_order = all(`,
which was causing the tests to pass.
However, the final expectation should test that we keep the same order
in the same session, and we were accidentally testing the opposite.
We're also adding an extra check to verify there are investments on the
page, since in some cases we were accessing pages with no investments,
and so these tests were always passing.
This is the actual number of investments per page in the index action.
Also note one test was generating 100 extra records, which made the test
take more than 40 seconds (on my machine).
We were showing only the ones being shown in the current page because
we were modifying `@investments` using a method which used
`@investments`, and we were calling that method twice.
There are many possible solutions: using a local variable to store the
result of the `investments` method, modifying `@investments` after
modifying `@investments_map_coordinates`, ... I've used the one which in
my humble opinion is a bit less fragile: not using `@investments` inside
the `investments` method. That way, the `investments` method will always
return the same result.
Note `stub_const("Budgets::InvestmentsController::PER_PAGE", 2)`
wouldn't work because `Budgets::InvestmentsController` isn't loaded when
that line is executed. So we need to load it. Instead of requiring the
file, using `#{Budgets::InvestmentsController}` seems to be an easier
solution.
