The resolution of most devices is at least 640 pixels in at least one of
their dimensions. Since we need the width to be smaller than 640 pixels,
having a height of 479px wasn't realistic.
Doing so caused the tests checking the sticky "support proposal" tests
to fail because on 480px-high devices the sticky message covers the
whole screen.
This is a usability issue, but since I'm not sure how many people use
such a small window when browsing the web, for now I'm simply changing
the tests.
We're changing the width to 320px since it's the width required by the
Web Content Accessibility Guidelines 2.2. Quoting these guidelines [1]:
> Content can be presented without loss of information or functionality,
> and without requiring scrolling in two dimensions for:
> * Vertical scrolling content at a width equivalent to 320 CSS pixels;
> * Horizontal scrolling content at a height equivalent to 256 CSS
> pixels.
Note that, using a height of 256px, the sticky message would make the
application unusable, though. IMHO we'll have to get rid of the sticky
message sooner or later.
We've been ignoring what the Bullet gem reports for at least 6 years
(maybe more), but we were still updating the gem and maintaining the
code in `config/environments/` (which caused conflicts every time we run
`rails app:update` to upgrade to a new Rails version). Maintaining it
isn't a huge effort, but it's infinitely bigger than the benefits we get
from it, which are zero.
Adding `includes` everywhere we query for records would be a huge
maintenance effort and would make the code less readable, so I don't
think it's worth it. We might do it occasionally if we detect a
performance bottleneck.
We could also use a gem to automatically avoid the N+1 queries problem,
like Goldiloader [1], ArLazyPreload [2] or JitPreload [3]. Benchmarks
show that the performance improvements obtained by using these gems is
about less than 10% (it depends a lot on the page being loaded, though),
which IMHO doesn't justify adding yet another gem that patches
ActiveRecord and that could be incompatible with other gems doing so.
There are a couple of open pull requests (at the time of writing,
they've been open for about two years) in the Rails repository [4][5] to
automatically avoid N+1 queries as well. For now, we'll hope something
similar is integrated in Rails itself in the future.
[1] https://github.com/salsify/goldiloader
[2] https://github.com/DmitryTsepelev/ar_lazy_preload
[3] https://github.com/clio/jit_preloader/
[4] Pull request 45231 in https://github.com/rails/rails/
[5] Pull request 45413 in https://github.com/rails/rails/
This rule was added in rubocop-rspec 2.19.0.
When freezing time in a test, `travel_back` is called automatically when
the test finishes, so we can do it in a `before` block instead of an
`around` block.
Note this rule didn't detect our usage of `freeze_time` because we were
using it on cops with a certain tag, but I expect the rule to be able to
detect this usage in the future.
Internet Explorer 8 was released in 2009 and people using it already
know that most web pages look broken on it, so we don't need to warn
them.
Removing it makes our application layout file much easier to read and
modify.
Creating a schema takes about 3-4 seconds on my machine, so omitting
the callbacks makes tests much faster.
To do so, we're using the `insert!` method added in Rails 6.0, which
inserts a record without executing callbacks or validations. To make the
tests look consistent, we're adding a FactoryBot strategy which uses
`insert!` instead of `create!`.
Note this strategy is useless in most cases because it doesn't work when
models have translatable attributes or associations. However, IMHO it's
worth it even if we only use it for tenants.
We could also use `Tenant.insert!` instead, but then we would have to
add all the mandatory attributes, and in this case the code is clearer
if we only add the attributes we need for the test.
On my machine, seeding a tenant takes about one second, so skipping this
action when it isn't necessary makes tests creating tenants faster
(although creating a tenant still takes about 3-4 seconds on my
machine).
Performance tests show both methods of truncating the database take
about the same time, so we can remove one dependency and we don't lose
anything in the process.
We were using this hack in order to allow `File.new` attachments in
tests files. However, we can use the `fixture_file_upload` helper
instead.
Just like it happened with `file_fixture`, this helper method doesn't
work in fixtures, so in this case we're using `Rack::Test::UploadedFile`
instead.
The `sign_in(nil)` method was a bit hard to understand IMHO. After all,
in controller and system tests we don't have to specify no user is
signed in; that's the default behavior.
So we're making it the default behavior for component tests as well.
The next warning is flooding the tests log:
`Options passed to Node#set but the RackTest driver
doesn't support any - ignoring`
As the rack test driver does not support any option, we
get rid of all `default_set_options` before each system
spec that uses this driver.
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 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.
For now we're keeping the test as it was, but eventually we'll have to
fix the bug.
We were testing what happens when users disable features in the admin
panel, so it makes sense to test what happens from the user's point of
view when trying to access a disabled feature: they see a page with the
test "Internal server error".
Whether we should responde with 500 Internal server error page or a 404
Not Found is up to debate; personally I find the latter more
appropriate.
Code based on the article "Changing Rails consider_all_requests_local in
RSpec fails" [1].
[1] http://atodorov.org/blog/2016/04/27/changing-rails-consider_all_requests_local-in-rspec-fails/
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.
Using GitHub Actions has a few advantages over using Travis CI:
* More jobs can be run in parallel
* All CONSUL repositories on GitHub will be configured automatically
Besides, Travis have recently changed their policy twice. First, they
announced their site for free software projects would be shut down but
free software projects could still use their site for private projects.
And then, they limited the usage of their services for free software
projects.
Just like we used to do with Travis, we're enabling builds for pull
requests but not for pushed branches.
We're also building the master branch. Even if we never push to the
master branch directly, we're aware other CONSUL repositories do, so
we're running the tests for this case.
By using real XML responses developers will be able to understand better
how the integration works (the data flow), and the correspondency between
`remote_census` settings and their place at a real XML response.
As `stubbed_responses` methods were removed from the model layer now the
stubbing part should be managed from the test environment code so also
added a new helper module `RemoteCensusSetup` that can be used anywhere
where we need to call the web service.
Co-Authored-By: Javi Martín <javim@elretirao.net>
By simplyfing the responses the configuration for specs can be simpler too.
We're also using more generic terms instead of the ones used in Madrid's
Census API.
Co-Authored-By: Javi Martín <javim@elretirao.net>
After upgrading to Turbolinks 5, redirects are followed on AJAX
requests, so we were accidentally redirecting the user after they mark
an investment as visible to valuators.
There was already a system spec failing due to this issue ("Admin budget
investments Mark as visible to valuators Keeps the valuation tags");
however, it only failed in some cases, so we're adding additional tests.
Ideally we would write a system test to check what happens when users
click on the checkbox. However, from the user's point of view, nothing
happens when they do so, and so testing it is hard. There's a usability
issue here (no feedback is provided to the user indicating the
investment is actually updated when they click on the checkbox and so
they might look for a button to send the form), which also results in a
feature which is difficult to test.
So we're writing two tests instead: one checking the controller does not
redirect when using a JSON request, and one checking the form submits a
JSON request.
I've chosen JSON over AJAX because usually requests to the update action
come from the edit form, and we might change the edit form to send an
AJAX request (and, in this case, Turbolinks would handle the redirect as
mentioned above).
Another option would be to send an AJAX request to a different action,
like it's done for the toggle selection action. I don't have a strong
preference for either option, so I'm leaving it the way it was. At some
point we should change the user interface, though; right now in the same
row there are two actions doing basically the same thing (toggling
valuator visibility and toggling selection) but with very different user
interfaces (one is a checkbox and the other one a link changing its
style depending on the state), resulting in a confusing interface.
All the code in the `bin/` and the `config/` folder has been generated
running `rake app:update`, except the `escape_javascript_fix` file,
which we've removed since the code there is already included in Rails
5.2.
The test wasn't working when postgres used the English dictionary
because in English the word "what" was ignored (or, at least, not given
enough relevance) while searching. When we wrote the test, it passed
because back then we always used the Spanish dictionary. However, when
we switched to a dictionary based on the default locale (in commit
d99875cd), we had to force this test to keep using the Spanish
dictionary.
Using the Spanish dictionary in a test where all texts are in English is
strange to say the least ;). So here we're making the test a bit easier
to understand.
Since now we're only using the `:spanish_search` tag in one test, I've
decided to remove it and simply add it to that test's setup.
With two concurrent requests, it's possible to create two ballot lines
when only one of them should be created.
The reason is the code validating the line is not thread safe:
```
if ballot.amount_available(investment.heading) < investment.price.to_i
errors.add(:money, "insufficient funds")
end
```
If the second request executes this code after the first request has
executed it but before the first request has saved the record to the
database, both records will pass this validation and both will be saved
to the database.
So we need to introduce a lock. Now when the second request tries to
lock the ballot, it finds it's already locked by the first request, and
will wait for the transaction of the first request to finish before
checking whether there are sufficient funds.
Note we need to disable transactions during the test; otherwise the
second thread will wait for the first one to finish.
Also note that we need to update a couple of tests because records are
reloaded when they're locked.
In one case, reloading the ballot causes `ballot.user` to be `nil`,
since the user is hidden. So we hide the user after creating all its
associated records (which is the scenario that would take place in real
life).
In the other case, reloading the ballot causes `ballot.user` to reload
as well. So we need to reload the user object used in the test too so it
gets the updates done on `ballot.user`.
I haven't been able to reproduce this behavior in a system test. The
following test works with Rails 5.0, but it stopped working when we
moved to system tests in commit 9427f014. After that commit, for reasons
I haven't been able to debug (reintroducing truncation with
DatabaseClaner didn't seem to affect this test, and neither did
increasing the number of threads in Puma), the two AJAX requests
executed here are no longer simultaneous; the second request waits for
the first one to finish.
scenario "Race conditions with simultaneous requests", :js do
allow_any_instance_of(Budget::Ballot::Line).to receive(:check_sufficient_funds) do |object|
allow(object).to receive(:check_sufficient_funds).and_call_original
object.check_sufficient_funds
sleep 0.3
end
["First", "Second"].each do |title|
create(:budget_investment, :selected,
heading: california,
price: california.price,
title: title
)
end
login_as(user)
visit budget_investments_path(budget, heading_id: california.id)
within(".budget-investment", text: "First") { click_link "Vote" }
within(".budget-investment", text: "Second") { click_link "Vote" }
expect(page).to have_link "Remove vote"
expect(Budget::Ballot::Line.count).to eq 1
end
We don't need to set this value. In commit f2ef27d3 I made a mistake
thinking `Globalize.locale` and `I18n.locale` should always be in sync,
but they're actually automatically in sync when `Globalize.locale` is
`nil`.
So the best way to avoid any issues is not to assign `Globalize.locale`,
and use `Globalize.with_locale` where necessary instead.
The method `available_locales` in
`RemoteTranslations::Microsoft::AvailableLocales` needs to execute a
request to an external server in order to work, meaning it will fail if
the machine its executed on doesn't have an internet connection.
So we're stubbing the method in the tests using it.
Since we use transactions now for every test, we can seed the database
at the beginning, and then it will go back to this state before a test
is executed.
Running the test suite is now considerably faster. On my machine, we
save a quarter of second per system test, meaning we save several
minutes for the whole suite.
Implementation tries to be open for further extensions, such as deciding on
search dictionary based on configuration option or by locale set for
given user.
We were inconsistent on this one. I consider it particularly useful when
a method starts with a `return` statement.
In other cases, we probably shouldn't have a guard rule in the middle of
a method in any case, but that's a different refactoring.
