Add specs from config refactor #139
Conversation
| @@ -1,6 +1,7 @@ | |||
| class InertiaShareTestController < ApplicationController | |||
| inertia_share name: 'Brandon' | |||
| inertia_share sport: -> { 'hockey' } | |||
| inertia_share({a_hash: 'also works'}) | |||
| end | ||
| end | ||
|
|
||
| context "when there is conditional data shared via before_action" do |
There was a problem hiding this comment.
Freezing the shared data prevents the before_action conditional sharing hack I cooked up! It's a really nice guard rail.
| @@ -154,4 +154,12 @@ | |||
| end | |||
| end | |||
| end | |||
|
|
|||
| describe 'a non existent route' do | |||
| it 'raises a 404 exception' do | |||
There was a problem hiding this comment.
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.
| protected | ||
|
|
||
| def conditionally_share_a_prop | ||
| self.class.inertia_share incorrectly_conditionally_shared_prop: 1 |
There was a problem hiding this comment.
genuine question: Why are we explicitly testing that this fails as opposed to just not testing it if it's undocumented behavior?
There was a problem hiding this comment.
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!
| it "raises an error because it is frozen" do | ||
| expect { | ||
| get conditional_share_show_with_a_problem_path, headers: {'X-Inertia' => true} | ||
| }.to raise_error(FrozenError) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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", "brandon@bellawatt.com", "eugene@bellawatt.com"] | |||
| spec.email = ["brian@bellawatt.com", "brandon@bellawatt.com", "eugene@bellawatt.com"] | |||
There was a problem hiding this comment.
It was a cool email, though 😄
There was a problem hiding this comment.
Maybe it should be my cooler alter ago 😅!
The back and forth on #121 revealed a couple places where we lacked test coverage. I created them locally as I reviewed.
This PR adds them in so Future Us have them available.