Mark slow specs with :slow tag#2764
Draft
lfdebrux wants to merge 5 commits into
Draft
Conversation
lfdebrux
force-pushed
the
ldeb-slow-specs
branch
6 times, most recently
from
51a6eef to
38b5f76
Compare
To help with comparing the time to run tests in CI and locally, add the `--profile` flag to the invocation of RSpec in the test workflow. Note that this shouldn't affect the actual runtime of the tests, because RSpec always records this information; the `--profile` flag just determines whether it is printed at the end or not [[1]]. [1]: https://github.com/rspec/rspec/blob/6687661ab4267307f89053b097d91155bed5dcbd/rspec-core/lib/rspec/core/example.rb#L284
On my machine (macOS Tahoe 26.4.1) when running request specs there are random delays that cause individual examples to sometimes take longer than 5 seconds. Profiling the specs showed that the `ViteRuby#dev_server_running?` check which occurs whenever a request is made was being held up by hostname resolution in the call to `Socket.tcp`. By default the dev server host is `localhost`; it is not clear why resolving localhost can sometimes be slow, it may be related to macOS/Bonjour, or something about the system configuration. Other developers have reported similar delays however. This commit adds a workaround, by changing the Vite configuration for the test profile to a hardcoded IP address, we remove the need for host resolution and the delay. This change doesn't make much of a difference to the total runtime of a complete test run, as a percentage of the total wall time, but does shave a little bit off, and also makes it easier to determine which tests are actually slow. Note: This delay occurs whether or not the dev server is running (it is not, for tests, unless run by the developer) and is unaffected by the configurable timeout delay in Vite. The delay was not present in CI because Vite is configured to assume that the dev server is not running in CI [1] without making a network call. [1]: https://github.com/ElMassimo/vite_ruby/blob/c6cf52a17199c50e7a09e3eea40f9a3d42986c0a/vite_ruby/lib/vite_ruby.rb#L110
Currently all our feature specs take over 1 second to run on my machine (MacBook Pro, Apple M1 Pro CPU, 16 GB RAM), as they rely on browser automation via headless Chrome. This commit adds a small bit of configuration to RSpec to add a `:slow` tag to all feature specs. This is partly for documentation purposes, but mainly it allows developers to skip slow specs when running tests locally with ``` rspec --tag ~slow ``` This change should not affect how the tests/PR checks run in CI.
Mark examples/example groups that consistently take over a second to run on my machine (MacBook Pro, Apple M1 Pro CPU, 16 GB RAM) with the `:slow` tag. This is partly for documentation purposes, but mainly it allows developers to skip slow specs when running tests locally with ``` rspec --tag ~slow ``` This change should not affect how the tests/PR checks run in CI. Feature specs are not marked in this commit, as they are already marked as slow generally (see the previous commit).
The `button` selector in Capybara when a locator is provided is very complicated [1], and for these examples it was taking around 5 seconds each on my machine (MacBook Pro, Apple M1 Pro CPU, 16 GB RAM). [1]: https://github.com/teamcapybara/capybara/blob/master/lib/capybara/selector/definition/button.rb
|
🎉 A review copy of this PR has been deployed! You can reach it at: https://pr-2764.admin.review.forms.service.gov.uk/ It may take 5 minutes or so for the application to be fully deployed and working. If it still isn't ready For the sign in details and more information, see the review apps wiki page. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this pull request solve?
On my machine (MacBook Pro, Apple M1 Pro, 16 GB RAM, macOS Tahoe 26.4.1), running the specs in this repo takes around 17 minutes.
This PR adds a
:slowtag to examples that consistently take over 1 second to run on my machine.This is partly for documentation purposes, but also allows developers to skip slow specs when running tests locally with
See the RSpec docs on the
--tagoption for details on how this works.The changes in this PR should not affect how the tests/PR checks run in CI.
On my machine, with the changes from this PR, running
rspec --tag ~slowtakes around 7 minutes.Notes on slow tests
While trying to figure what to tag as slow, I found three different kinds of slow tests:
Anything that uses headless Chrome for testing is automatically a resource hog; I didn't look too closely at these. Instead I just added a bit of configuration to make all feature specs be marked as slow.
In theory we could try and speed some feature specs up by seeing which ones require a real browser and which could use the RackTest simulator that comes with Capybara [1]. However, in practice, most of our feature specs require a real browser because they a) rely on JavaScript for the sign in journey, and b) test for WCAG failures with aXe tools, which requires Chrome. So it would be quite a lot of work to try and speed up our feature specs this way (although it might become worth it at some point).
There were a few specs that were consistently significantly slower than a second when run on their own. These mostly appear to be weird outliers, and there also aren't that many of them. I've fixed two (see commit messages for details), and left the rest marked with the
:slowtag.This was the biggest category of issue I found. When repeatedly running tests with
rspec --profile --tag ~slow, after having marked tests in the above two categories, I found there was little consistency between runs in which tests were in the top ten slowest list.After doing some profiling with ruby-prof using a modified version of https://gist.github.com/palkan/73395cc201a565ecd3ff61aac44ad5ae, I found one issue related to ViteRuby and added a workaround for that (see commit messages for details), but adding that workaround did not fully resolve the intermittent slowness. I also found that some specs can be slow when run in isolation, but quick when run as part of a group, likely because there is some shared caching happening.
Basically, some of the slowness is likely caused by a combination of Factory Bot (a known issue generally), bootsnap caching, thread contention, and general operating system constraints, and these are all difficult to pin down and fix.
Things to consider when reviewing