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.
This is a mistake I made in commit f2ef27d3. Back then I thought we
needed to keep Globalize.locale and I18n.locale in sync, but the truth
is it automatically happens when setting Globalize.locale to nil.
So now we can use I18n.with_locale (at least in the tests) and forget
about Globalize, which will make it easier to switch to Mobility in the
future.
We already configure `I18n.locale` and we reset Globalize's fallbacks
before every test.
On the other hand, RSpec automatically resets anything which is stub
with `allow`, so there's no need to use `and_call_original` in an
`after` block.
The images from OpenStreetMap take a while to load, sometimes even
causing Net::ReadTimeout errors if the internet connection is slow. It's
happened a lot recently on Travis builds.
Using capybara-webmock we guarantee the test suite doesn't fail due to
network issues.
The dates are saved on UTC times on the database. So, for example,
if living in West Australia, `Date.current.beginning_of_day` will be
stored as UTC's yesterday at 15:15:00, while `Date.current.end_of_day`
will be stored as UTC's today at 15:14:59.
When we use the `DATE` database function, PostgreSQL will select the
records with the same UTC date as the current UTC date. However, we need
the records with the same application date (as defined in
`config.time_zone`) as the current application date. The test passed
(for us) because we were using `beginning_of_day + 3.hours` to make sure
we were creating records when the date in Madrid was the same as the UTC
date.
Using a ruby interval for the time condition solves the problem.
When any helper, lib, mailer, model or view spec is executed
after a feature, controller or request spec Globalize.fallbacks
returns nil and this can cause some flaky specs. With this
patch we are ensuring to initialize Globalize fallbacks
between specs.
Controller, feature and request specs do not need this patch
because of application_controller is currently initializing
Globalize.fallback on each request.
Resetting sessions and driver is automatically done by requiring
'capybara/rspec', as shown by the (lack of) that configuration for RSpec
in the Capybara README, manual testing of those settings, and Capybara's
code itself.
Using `Date.today` caused some milestones to be published before/after
the date defined by `Rails.application.config.time_zone`.
See also commit AyuntamientoMadird/consul@088c76d for a more detailed
explanation.
The test "Budget Investments Show milestones" was failing in certain
cases where `Globalize.locale` had been changed in a previous test.
Since having different values in `Globalize.locale` and `I18n.locale`
has proven to be an issue on the test enviroment, this commit also
changes application code in order to avoid similar situations on
production.
See issue #2718.
