Skip to content

Add specs from config refactor #139

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.

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 6 commits into from
Oct 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion inertia_rails.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Gem::Specification.new do |spec|
spec.name = "inertia_rails"
spec.version = InertiaRails::VERSION
spec.authors = ["Brian Knoles", "Brandon Shar", "Eugene Granovsky"]
spec.email = ["brain@bellawatt.com", "[email protected]", "[email protected]"]
spec.email = ["brian@bellawatt.com", "[email protected]", "[email protected]"]
Copy link
Contributor

Choose a reason for hiding this comment

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

It was a cool email, though 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe it should be my cooler alter ago 😅!


spec.summary = %q{Inertia adapter for Rails}
spec.homepage = "https://github.com/inertiajs/inertia-rails"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
class InertiaConditionalSharingController < ApplicationController
before_action :conditionally_share_a_prop, only: :show_with_a_problem

inertia_share normal_shared_prop: 1

inertia_share do
Expand All @@ -16,4 +18,16 @@ def show
show_only_prop: 1,
}
end

def show_with_a_problem
render inertia: 'EmptyTestComponent', props: {
show_only_prop: 1,
}
end

protected

def conditionally_share_a_prop
self.class.inertia_share incorrectly_conditionally_shared_prop: 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

genuine question: Why are we explicitly testing that this fails as opposed to just not testing it if it's undocumented behavior?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I first wrote a variation of this test, it didn't fail. It actually passed. Before #121 , this code would not conditionally share that prop. Because the inertia_share store is a class instance variable, it would be included on all subsequent requests after the first time it ran.

It's not very elegant code, but I think the use case is valid. If I can share data per-controller, why not per action? I wrote the original spec just to document the behavior.

However, @ElMassimo 's refactor actually prevents this from occurring at all because it freezes the shared data structure once the controller class is loaded into memory. And that is (part of) the intended behavior of his change. So, the spec now documents/ensures you can't even try the contrived workaround for per-action data sharing. To me, this is a nice guardrail that fails loudly, and since I was surprised by the new behavior when I added the spec back, I think it's worth keeping!

end
end
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
class InertiaShareTestController < ApplicationController
inertia_share name: 'Brandon'
inertia_share sport: -> { 'hockey' }
inertia_share({a_hash: 'also works'})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@skryukov pointed this one out in #121

inertia_share do
{
position: 'center',
Expand Down
2 changes: 1 addition & 1 deletion spec/dummy/config/environments/test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
config.cache_store = :null_store

# Raise exceptions instead of rendering exception templates.
config.action_dispatch.show_exceptions = :none
config.action_dispatch.show_exceptions = false

# Disable request forgery protection in test environment.
config.action_controller.allow_forgery_protection = false
Expand Down
1 change: 1 addition & 0 deletions spec/dummy/config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,5 @@

get 'conditional_share_index' => 'inertia_conditional_sharing#index'
get 'conditional_share_show' => 'inertia_conditional_sharing#show'
get 'conditional_share_show_with_a_problem' => 'inertia_conditional_sharing#show_with_a_problem'
end
42 changes: 22 additions & 20 deletions spec/inertia/conditional_sharing_spec.rb
Original file line number Diff line number Diff line change
@@ -1,28 +1,30 @@
# Specs as documentation. Per-action shared data isn't explicity supported,
# but it can be done by referencing the action name in an inertia_share block.
RSpec.describe "conditionally shared data in a controller", type: :request do
context "when there is conditional data inside inertia_share" do
it "does not leak data between requests" do
get conditional_share_index_path, headers: {'X-Inertia' => true}
expect(JSON.parse(response.body)['props'].deep_symbolize_keys).to eq({
index_only_prop: 1,
normal_shared_prop: 1,
})

# NOTE: we actually have to run the show action twice since the new implementation
# sets up a before_action within a before_action to share the data.
# In effect, that means that the shared data isn't rendered until the second time the action is run.
get conditional_share_show_path, headers: {'X-Inertia' => true}
context "when there is data inside inertia_share only applicable to a single action" do
it "does not leak the data between requests" do
get conditional_share_show_path, headers: {'X-Inertia' => true}
expect(JSON.parse(response.body)['props'].deep_symbolize_keys).to eq({
normal_shared_prop: 1,
show_only_prop: 1,
conditionally_shared_show_prop: 1,
})
normal_shared_prop: 1,
show_only_prop: 1,
conditionally_shared_show_prop: 1,
})

get conditional_share_index_path, headers: {'X-Inertia' => true}
expect(JSON.parse(response.body)['props'].deep_symbolize_keys).to eq({
index_only_prop: 1,
normal_shared_prop: 1,
})
expect(JSON.parse(response.body)['props'].deep_symbolize_keys).not_to include({
conditionally_shared_show_prop: 1,
})
end
end

context "when there is conditional data shared via before_action" do
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Freezing the shared data prevents the before_action conditional sharing hack I cooked up! It's a really nice guard rail.

it "raises an error because it is frozen" do
# InertiaSharedData isn't frozen until after the first time it's accessed.
InertiaConditionalSharingController.send(:_inertia_shared_data)

expect {
get conditional_share_show_with_a_problem_path, headers: {'X-Inertia' => true}
}.to raise_error(FrozenError)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does anyone by chance know why Rails 7.0 and lower doesn't raise an error here while Rails 7.1 does?

My guess is that maybe there was a change in the way Rails loads the code in 7.1, and previous versions don't actually load/freeze the data in the controller before this line gets called?

Copy link
Contributor

Choose a reason for hiding this comment

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

We use Rails 7.1 config.action_dispatch.show_exceptions = :none config option for the dummy app, which evaluates to just truthy in Rails < 7.1 and acts like the :all option:
https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#new-ways-to-handle-exceptions-in-controller-tests-integration-tests-and-system-tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, that was part of it but there was a second subtler bug. Because the shared data is frozen the first time you access the shared data, if the first request to a controller has one of these (contrived) conditionally shared data examples, then it won't raise an error.

So, in the test suite, the failure (to raise a FrozenArray exception) was intermittent, depending on which spec in that file ran first.

It's an edge case of an edge case, so I don't feel we need to prevent it; and I still like having a spec to document the frozen data behavior.

In any case, thanks for #140 , it gave me a headstart tonight once I got back to looking at this one!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, good catch. What do you think about marking inertia_share and inertia_config as protected? This way, calling them from action will raise an error, which might be a clear signal to users that they're doing something wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, that might be cleaner. I'm not super worried about it either way now. It'll raise the FrozenArray error the second time you try to call the conditional action, so the failure I was seeing was just one of those test suite not quite matching reality quirks.

The per-action use case isn't actually officially supported at the moment, so we're being helpfully defensive enough already, IMO.

end
end
end
8 changes: 8 additions & 0 deletions spec/inertia/request_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -154,4 +154,12 @@
end
end
end

describe 'a non existent route' do
it 'raises a 404 exception' do
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Arguably this is just testing Rails instead of InertiaRails, but I wrote this to reproduce a possible bug that @skryukov found in the middleware, so I'm leaving it in.

expect {
get '/non_existent_route', headers: { 'X-Inertia' => true }
}.to raise_error(ActionController::RoutingError, /No route matches/)
end
end
end
8 changes: 4 additions & 4 deletions spec/inertia/sharing_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
subject { JSON.parse(response.body)['props'].deep_symbolize_keys }

context 'using inertia share' do
let(:props) { {name: 'Brandon', sport: 'hockey', position: 'center', number: 29} }
let(:props) { {name: 'Brandon', sport: 'hockey', position: 'center', number: 29, a_hash: 'also works'} }
before { get share_path, headers: {'X-Inertia' => true} }

it { is_expected.to eq props }
Expand All @@ -18,7 +18,7 @@
end

context 'using inertia share in subsequent requests' do
let(:props) { {name: 'Brandon', sport: 'hockey', position: 'center', number: 29} }
let(:props) { {name: 'Brandon', sport: 'hockey', position: 'center', number: 29, a_hash: 'also works'} }

before do
get share_path, headers: {'X-Inertia' => true}
Expand All @@ -29,7 +29,7 @@
end

context 'using inertia share with inheritance' do
let(:props) { {name: 'No Longer Brandon', sport: 'hockey', position: 'center', number: 29} }
let(:props) { {name: 'No Longer Brandon', sport: 'hockey', position: 'center', number: 29, a_hash: 'also works'} }

before do
get share_with_inherited_path, headers: {'X-Inertia' => true}
Expand All @@ -39,7 +39,7 @@
end

context 'with errors' do
let(:props) { {name: 'Brandon', sport: 'hockey', position: 'center', number: 29} }
let(:props) { {name: 'Brandon', sport: 'hockey', position: 'center', number: 29, a_hash: 'also works'} }
let(:errors) { 'rearview mirror is present' }
before {
allow_any_instance_of(ActionDispatch::Request).to receive(:session) {
Expand Down