Skip to content

Cache ActionView::FixtureResolvers created by stub_template #1979

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 5, 2018
Merged

Cache ActionView::FixtureResolvers created by stub_template #1979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
392c35c
Add test for ViewExampleGroup#stub_template
urbanautomaton Mar 30, 2018
3666c35
Cache stub_template resolvers between example groups
urbanautomaton Mar 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/rspec/rails/example/view_example_group.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,14 @@ module ViewExampleGroup
include RSpec::Rails::ViewAssigns
include RSpec::Rails::Matchers::RenderTemplate

# @private
module StubResolverCache
def self.resolver_for(hash)
@resolvers ||= {}
@resolvers[hash] ||= ActionView::FixtureResolver.new(hash)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern is that this introduces shared state across tests (I know this is by design) but if we were to freeze this object it wouldn't be mutable which would alleviate this concern

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that's a straightforward change to make, have pushed with the extra .freeze. 👍

However, there's some further mutable internal state in the resolver that I don't think we'll be able to freeze; it only builds and caches Template instances when it receives queries for a specific path, so this happens on first render, not when we're instantiating the resolver.

My intuition is that this shared state should be safe, though. Since we're indexing the cache on the full stub definition (i.e. filename and stubbed contents), and the output of a compiled template should be a function of its contents, I don't think there's any way for the cache to return incorrect values.

end
end

# @private
module ClassMethods
def _default_helper
Expand Down Expand Up @@ -84,7 +92,7 @@ def view
#
# stub_template("widgets/_widget.html.erb" => "This content.")
def stub_template(hash)
view.view_paths.unshift(ActionView::FixtureResolver.new(hash))
view.view_paths.unshift(StubResolverCache.resolver_for(hash))
end

# Provides access to the params hash that will be available within the
Expand Down
31 changes: 31 additions & 0 deletions spec/rspec/rails/example/view_example_group_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,5 +254,36 @@ def _view; end
view_spec.template
end
end

describe '#stub_template' do
let(:view_spec_group) do
Class.new do
include ViewExampleGroup::ExampleMethods
def _view
@_view ||= Struct.new(:view_paths).new(['some-path'])
end
end
end

it 'prepends an ActionView::FixtureResolver to the view path' do
view_spec = view_spec_group.new
view_spec.stub_template('some_path/some_template' => 'stubbed-contents')

result = view_spec.view.view_paths.first

expect(result).to be_instance_of(ActionView::FixtureResolver)
expect(result.hash).to eq('some_path/some_template' => 'stubbed-contents')
end

it 'caches FixtureResolver instances between example groups' do
view_spec_one = view_spec_group.new
view_spec_two = view_spec_group.new

view_spec_one.stub_template('some_path/some_template' => 'stubbed-contents')
view_spec_two.stub_template('some_path/some_template' => 'stubbed-contents')

expect(view_spec_one.view.view_paths.first).to eq(view_spec_two.view.view_paths.first)
end
end
end
end