Halve the time it takes to run view specs by enabling the resolver cache #1452
Conversation
Rails expects value of the `:format` option be a symbol. It filters formats against `Mime::SET.symbols` so formats which are unknown (or are known but are of an unexpected type) are not used in cache keys for the Rails' `ActionView::Resolver::Cache`. This made the resolver cache unusable when there was multiple templates with the same name but different formats and was the reason the cache was disabled in rspec-rails [1]. This isn't the only way of fixing this problem- for example Rails could be changed to ensure symbols aren't compared with strings as was suggested in [2], but I feel that it's rspec-rails' responsibility to work with the existing Rails API. [1] rspec#864 [2] rails/rails#13461
The caching was turned off in [1] because different formats were being cached under the same key. I believe the need to do this was removed in the previous commit. This reduces the time it takes to run FutureLearn’s view specs by 50%. I think it’s sensible to use the view resolver cache during tests because it’s normally on between requests in other environments including when creating tests with `ActionView::TestCase` in a new Rails application. It looks like it was only turned off in rspec-rails to work around this caching issue. I haven't added any test coverage in this change since it feels like it would be testing a negative (the resolver cache isn't cleared). [1] rspec#864
Vaguely, this rings a bell, I believe the reason why we didn't just do this at the time is rails treats views with strings and symbols differently |
|
@JonRowe thanks for taking a look at this,
I couldn't see any reference to this in the original commit or subsequent discussion that suggests that there would be a problem marking the value of I have looked myself and could only think of an issue when a user of rspec-rails has registered multiple custom mime types with a string rather than a symbol, e.g: Mime::Type.register "audio/mp3", "mp3"
Mime::Type.register "audio/mp4", "mp4"This change would cause the resolver cache to mix templates during tests in this case. It looks to me from the |
|
/cc @cupakromer |
|
Seems like this originated from 96d3e75 - if the cukes are passing now perhaps it's no longer necessary? Would this break view specs for people using some outdated version of rails that has the invalidation issue that @JonRowe referenced? I don't know… but if they're upgrading their RSpec and staying on a legacy version of rails I think that they should probably use an older rspec-rails until they update their rails since it sounds like more of a bug on rails' side. So I think I've talked myself into 👍 |
on this issue.
Halve the time it takes to run view specs by enabling the resolver cache
|
@soulcutter thanks for your time spent reviewing this and merging it in. In case anyone else has a lot of view specs and wants to take advantage of this change now (but doesn't want the other changes on You can use this by changing the gem 'rspec-rails', git: 'https://github.com/Futurelearn/rspec-rails.git', branch: 'faster-view-specs-backport' |
Hi,
Recently at FutureLearn we have taken some time to try and speed up our tests.
We have lots of view specs in our test suite, and we noticed that they took quite a while to run. This surprised us given that one of the benefits of testing views independently is speed. We found that one of the reasons for this slowness was that rspec-rails had disabled the ActionView resolver cache, increasing the time it took to load views.
There are two commits in this pull request. The first contains a fix the original issue which caused the cache to be disabled, the second enables the cache again. Please see their respective commit messages for more detail. When running our tests against this branch, it halves the time it takes us to run our view specs.
I'd like to hear peoples thoughts on this change, particularly: