Expose path to view specs #1402

sarahmei · 2015-06-18T19:55:16Z

Fix for #1347.

Expose inferred path data to view specs, since we already expose inferred controller & action. This will allow view specs to exercise views that depend on the current page.

We did two implementations - the first commit uses rescue nil - the second one has no rescues but uses somewhat awkward logic to iterate through the routes to find a matching one.

If you know of a better way to implement this without rescue...would love to hear about it.

fables-tales · 2015-06-22T18:55:30Z

lib/rspec/rails/view_controller.rb

+  module Rails
+    # Methods added to ActionView::TestCase::TestController, only in view specs
+    module ViewController
+      def self.included(_klass)


Unless I'm mistaken, this will permanently include the ExtraParamsAccessors methods into ActionView::TestCase::TestController for the rest of the process, including other types of specs. Is my read on this right, and is that the intended behaviour? To me, this looks like it's basically lazy addition of some methods to ActionView::TestCase::TestController as opposed to only including some methods in to view specs. If that's the intention, then does the comment need to be changed to better reflect that's whats happening?

PS: If I'm wrong, please let me know, I'm not super familiar with this code ^_^

I hadn't thought about that distinction. The intention is to include them only for view specs. It's an awkward fit because the user has to set them somehow, and they've already got a controller object available. I thought about surfacing a whole different object, but the extra params are not unrelated to the controller, request, etc.

That said - what's the right way to include those methods in the controller class just for view specs?

@cupakromer would know better than me, but my instincts are saying before/after that adds/remove methods in a similar way to how syntax.rb works: https://github.com/rspec/rspec-expectations/blob/master/lib/rspec/expectations/syntax.rb#L38-L64

fables-tales · 2015-06-22T18:57:32Z

I've left a few comments. @cupakromer if you agree/have further questions I'd love if you could take a look at this when you get a chance.

fables-tales · 2015-06-22T18:57:59Z

Open question: does this need to be squashed before merging?

JonRowe · 2015-06-23T09:24:31Z

I'd like it if we could get this squashed :)

cupakromer · 2015-06-24T03:12:07Z

features/view_specs/view_spec.feature

+      end
+      """
+    When I run `rspec spec/views`
+    Then the examples should all pass


Yay cukes! 💙 I recognize that the style for these mirrors the existing spec. If you'd be so kind to update them with a newer style we working towards that would be great!

Use backticks for code in descriptions (including scenarios) as in normal markdown, so the scenarios should use request.fullpath

Scenarios should start capitalized; does not apply when starting with a code fragment

Avoid "should" opting for more direct and present language. For example "Actions which do not require extra parameters have request.fullpath set"

This is resolved, and it's a hangover comment that github didn't squash.

cupakromer · 2015-06-24T04:07:30Z

I like how this is progressing. 😸 : I like seeing the progress over the commit history. I'd really like to persevere a bunch of that if we can. I think when doing the final squash, if the ideas, the why of the current design decisions, and details about attempted but ultimately removed design decisions would be awesome 💙 💙

In general I strongly avoid using rescue for logic flow. I know @sarahmei and I talked about this a bit on IRC before I went MIA being all 😷 😴. Coming back to this with fresh eye, I'm wondering if rescue may actually be the best strategy in this case. My main concern is legacy and monolith apps. I do not have any hard numbers currently (will ask around tomorrow), but I'm thinking of those apps with possibly > 100 routes.

My thinking goes:

url_for is called all over the place in Rails; so they need to do that look up a lot and I feel better assuming they've optimize the logic better than I could or will (I haven't tested this code ❤️ against theirs so this could be better logic 😄)
we call url_for either way, in the rare edge case where we may have missed some logic in our lookup, we'll still get the exception; which we are no longer rescuing

I'm happy to talk to some people tomorrow and get some hard numbers on larger apps and their route count. I'm also happy to test out a benchmark using rescue and this lookup to see which is faster.

sarahmei · 2015-07-10T18:23:59Z

@imryo and I are working on a new branch that does this with a rescue instead of iterating, and also incorporates these suggestions.

sarahmei · 2015-07-12T21:34:44Z

@cupakromer @samphippen what's your recommendation for dealing with code that's different in 1.8.7 vs 1.9? instance_methods has string keys in 1.8.7 and symbol keys in 1.9+. 😦

fables-tales · 2015-07-12T22:15:13Z

.find {|x| x.to_sym == the_thing}.nil? is probably the best I can come up with right now. @cupakromer thoughts?

myronmarston · 2015-07-12T22:37:06Z

We often conditionally define one of two method implementations based on a Ruby version. Would that work here? (On my phone so hard to link you to examples right now)

myronmarston · 2015-07-12T22:39:12Z

Actually, looking at the include check, I think that 'klass.method_defined?(name)' would work best. It works with strings or symbols on 1.8 and 1.9+ and is constant time instead of linear.

sarahmei · 2015-07-13T00:03:00Z

Thanks @myronmarston! method_defined? worked on all the rubies. @cupakromer this is ready to look at again. :)

JonRowe · 2015-07-20T11:17:30Z

Ping @cupakromer, did you get a chance to rereview this?

cupakromer · 2015-07-20T15:34:07Z

Sorry, slipped off my todo list. I will get to this today :-D

cupakromer · 2015-07-21T14:55:19Z

.gitignore

@@ -21,3 +21,4 @@ bundle
 .bundle
 specs.out
 spec/examples.txt
+specs.out


ryanclark2 · 2015-09-15T19:31:56Z

@cupakromer I think we have one more comment to straighten out before this can be accepted. It would be great to finish this up.

This enables URL helpers to work in view specs via the `extra_params` API. This API updates the controller's path information via a writer and allows the user to read path information out of the controller.

Expose path to view specs

Guess people forget this when working on rspec#1402 rspec#1402

Guess people forget this when working on #1402 #1402

Guess people forget this when working on rspec#1402 rspec#1402

fables-tales reviewed Jun 22, 2015
View reviewed changes

cupakromer reviewed Jun 24, 2015
View reviewed changes

cupakromer reviewed Jul 21, 2015
View reviewed changes

.gitignore

@@ -21,3 +21,4 @@ bundle

.bundle

specs.out

spec/examples.txt

specs.out

Copy link

Member

cupakromer Jul 21, 2015

Choose a reason for hiding this comment

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

❤️

fables-tales force-pushed the expose_path_to_view_specs branch 2 times, most recently from a6a2df2 to 527605f Compare December 16, 2015 15:23

fables-tales pushed a commit to sarahmei/rspec-rails that referenced this pull request Dec 16, 2015

Add a changelog entry for rspec#1402

aa735b5

ryanclark2 and others added 2 commits December 16, 2015 15:51

Fix URL helpers via the extra_params API to view specs.

51cc863

This enables URL helpers to work in view specs via the `extra_params` API. This API updates the controller's path information via a writer and allows the user to read path information out of the controller.

Add a changelog entry for rspec#1402

adf9a4d

fables-tales force-pushed the expose_path_to_view_specs branch from aa735b5 to adf9a4d Compare December 16, 2015 15:51

fables-tales pushed a commit that referenced this pull request Dec 17, 2015

Merge pull request #1402 from sarahmei/expose_path_to_view_specs

e566410

Expose path to view specs

fables-tales merged commit e566410 into rspec:master Dec 17, 2015

fables-tales deleted the expose_path_to_view_specs branch December 17, 2015 14:17

fables-tales mentioned this pull request Apr 2, 2016

view specs doesn't render "current_page?" condition? #1347

Closed

hlcfan added a commit to hlcfan/rspec-rails that referenced this pull request May 11, 2017

Correct return value of path_for

a751cb5

Guess people forget this when working on rspec#1402 rspec#1402

hlcfan mentioned this pull request May 11, 2017

Correct return value of path_for #1815

Merged

hlcfan added a commit to hlcfan/rspec-rails that referenced this pull request May 11, 2017

Correct return value of path_for in documentation

d32b51b

Guess people forget this when working on rspec#1402 rspec#1402

hlcfan added a commit to hlcfan/rspec-rails that referenced this pull request May 11, 2017

Correct return value of path_for in documentation

807b8ef

Guess people forget this when working on rspec#1402 rspec#1402

JonRowe pushed a commit that referenced this pull request May 13, 2017

Correct return value of path_for in documentation (#1815)

442f6d1

Guess people forget this when working on #1402 #1402

sebjacobs pushed a commit to futurelearn/rspec-rails that referenced this pull request Mar 15, 2019

Add a changelog entry for rspec#1402

a7d19c1

sebjacobs pushed a commit to futurelearn/rspec-rails that referenced this pull request Mar 15, 2019

Correct return value of path_for in documentation (rspec#1815)

74843dd

Guess people forget this when working on rspec#1402 rspec#1402

Uh oh!

Expose path to view specs #1402

Expose path to view specs #1402

Uh oh!

Conversation

sarahmei commented Jun 18, 2015

Uh oh!

fables-tales Jun 22, 2015

Choose a reason for hiding this comment

Uh oh!

sarahmei Jun 22, 2015

Choose a reason for hiding this comment

Uh oh!

fables-tales Jun 22, 2015

Choose a reason for hiding this comment

Uh oh!

fables-tales commented Jun 22, 2015

Uh oh!

fables-tales commented Jun 22, 2015

Uh oh!

JonRowe commented Jun 23, 2015

Uh oh!

cupakromer Jun 24, 2015

Choose a reason for hiding this comment

Uh oh!

fables-tales Dec 16, 2015

Choose a reason for hiding this comment

Uh oh!

cupakromer commented Jun 24, 2015

Uh oh!

sarahmei commented Jul 10, 2015

Uh oh!

sarahmei commented Jul 12, 2015

Uh oh!

fables-tales commented Jul 12, 2015

Uh oh!

myronmarston commented Jul 12, 2015

Uh oh!

myronmarston commented Jul 12, 2015

Uh oh!

sarahmei commented Jul 13, 2015

Uh oh!

JonRowe commented Jul 20, 2015

Uh oh!

cupakromer commented Jul 20, 2015

Uh oh!

cupakromer Jul 21, 2015

Choose a reason for hiding this comment

Uh oh!

ryanclark2 commented Sep 15, 2015

Uh oh!

Uh oh!