This rule was added in rubocop-capybara 2.19.0. We were following it
about 85% of the time.
Now we won't have to check both have_css and have_selector when
searching the code.
Using the `document` or `documents` classes meant styles defined for the
public list of documents conflict with these ones.
So now we're using HTML classes that match the name of the Ruby
component classes, as we usually do.
We applied the Capybara/SpecificMatcher in commit f52a86b46. However,
this rule doesn't convert methods finding <a> tags to methods finding
links because <a> tags only count as links when they've got the `href`
attribute. For instance, in the `xss_spec.rb` file we check what happens
when clicking on an anchor tag because we're testing that the `href`
attribute has been removed and so we can't use `click_link`.
So, basically, we can't enable a rule to automatically detect when we're
using `have_css` instead of `have_link`, but we should still do it
because `have_link` adds an extra check which affects accessibility
since it makes sure the tag has the `href` attribute and so it's
recognizable as a link by screen readers.
There were cases where we clicked the button to submit the form and
immediately we visited a different page.
In the past, we've had similar code produce PG::ProtocolViolation errors
in similar situations. Since we've had these errors a few times in the
nested imageable specs, there's a chance they're related to the absence
of the expectation.
Although I'm not even remotely sure this will fix these issues, at least
we now follow the convention of making expectations after every request.
Note we're changing both the nested imageable and nested documentable
specs. Only the nested imageable would need to be changed because it's
the one where there's a `visit` inside the
`imageable_redirected_to_resource_show_or_navigate_to` method. I'm
changing both for consistency.
Since we were creating a new answer in the form, we weren't getting the
errors associated to the answer the administrator was trying to create,
and so we were skipping the test.
Using the answer which contains the information about validation errors
fixes the issue and so we don't have to skip the tests.
This way the tests won't appear as "pending" when running the test
suite, and so we get rid of a lot of noise in the test results. There
doesn't seem to be a way to call `skip` without the test being marked as
"pending".
Note that in the globalizable tests we need to build a factory before
deciding whether an atribute is required or not (particularly for the
milestone factory, since milestone attributes are required depending on
the presence of other attributes). This isn't possible before we're
inside the test, so we can't add an `if:` condition to the test. So
we're adding the condition inside the test instead. A minor
inconvenience of this method is the test still runs even when the
condition is `false`.
This way we don't have to write `"spec/fixtures/files"` every time.
Note this method isn't included in factories. We could include it like
so:
```
FactoryBot::SyntaxRunner.class_eval do
include ActiveSupport::Testing::FileFixtures
self.file_fixture_path = RSpec.configuration.file_fixture_path
end
```
However, I'm not sure about the possible side effects, and since we only
use attachments in a few factories, there isn't much gain in applying
the monkey-patch.
This fixes a few issues we've had for years.
First, when attaching an image and then sending a form with validation
errors, the image preview would not be rendered when the form was
displayed once again. Now it's rendered as expected.
Second, when attaching an image, removing it, and attaching a new
one, browsers were displaying the image preview of the first one. That's
because Paperclip generated the same URL from both files (as they both
had the same hash data and prefix). Browsers usually cache images and
render the cached image when getting the same URL.
Since now we're storing each image in a different Blob, the images have
different URLs and so the preview of the second one is correctly
displayed.
Finally, when users downloaded a document, they were getting files with
a very long hexadecimal hash as filename. Now they get the original
filename.
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).
We introduced a bug in commit acbd1b023.
When editing a record and removing an existing image, we don't remove
the HTML fields associated with that image but simply hide them, and
then we add fields to create a new image when clicking on "Add image".
This is standard cocoon behavior. However, since in the case of images
there's a `has_one` relation, cocoon doesn't add unique identifiers to
the new fields, generating duplicate IDs, which is invalid HTML.
Since there's a duplicate file input ID, clicking on the "Choose image"
label we aren't clicking on the new input but on the old one. This means
we aren't correctly attaching an image. The tests passed because
Capybara uses the equivalent of a keyboard to select the field, and in
this case everything worked properly.
So we need to delete the existing elements before inserting new ones.
We're adding a test to check there aren't duplicate IDs.
We were hiding the file input and styling the label as a button instead.
Since clicking on a label has the same effect as clicking on the input,
the input worked properly for mouse and touch screen users.
However, hiding the input makes it inaccessible for keyboard users,
since labels don't get keyboard focus, but inputs do.
So we must not hide the input but make it invisible instead. But we
still need to hide the input (alongside the label) after a file has been
attached.
We could add some extra JavaScript to hide the input when we hide the
label. Since the JavaScript is already quite complex and my first few
attempts at changing it failed, I've opted to assume that the input (and
its label) must be hidden whenever there's already a file name, and
implement that rule with CSS.
Note we're using the `:focus-within` pseudoclass to style a label when
focus is on the input. This rule (at the time of writing) is only
supported by 93.5% of the browsers. Keyboard users without a screen
reader and using the other 6.5% of the browsers will still be able to
focus on the field but might not notice the field has received focus.
Since the percentage of affected users will decrease over time and until
now 100% of keyboard users were completely unable to focus on these
fields, for now we think this is a good-enough solution.
In the Management section when creating an investment we were not passing the
images attributes, so we were never able to associate images.
Make the nested_imageable spec compatible with the Management section.
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.
This rule was added in rubocop-rspec 1.39.0. The `visible: false` option
is equivalent to `visible: :all`, but we generally use it meaning
`visible: :hidden` for positive expectations and `visible: :all` for
negative expectations.
The only exceptations are tests where we count the number of map icons
present. I'm assuming in this case we care about the number of map icons
independently on whether they are currently shown in the map, so I'm
keeping the `visible: :all` behavior in this case.
By default, Capybara only finds visible elements, so adding the
`visible: true` option is usually redundant.
We were using it sometimes to make it an obvious contrast with another
test using `visible: false`. However, from the user's perspective, we
don't care whether the element has been removed from the DOM or has been
hidden, so we can just test that the visible selector can't be found.
Besides, using `visible: false` means the test will also pass if the
element is present and visible. However, we want the test to fail if the
element is visible. That's why a couple of JavaScript-dependant tests
were passing even when JavaScript was disabled.
We were submitting the form without checking the AJAX request to attach
the image had finished, so sometimes two requests were executed at the
same time. Sometimes this made InvisibleCaptcha to go crazy and report
the form was submitted too quickly.
Checking the first AJAX request has finished before submitting the form
solves the problem.
