Rails Ruby on Rails → 3.0.2

Comments and changes to this ticket

• You flagged this item as spam.
  

  Elad Meidar September 26th, 2009 @ 07:25 PM

  I've attached a patch that ports ActiveView:TestCase to master

  • LH3260-ActionView_TestCase_ported_to_master.diff 14.5 KB

• You flagged this item as spam.
  

  Erik Ostrom September 26th, 2009 @ 09:12 PM

  Patch doesn't apply cleanly to master. There are a couple of simple class renames in the way, and then a more complex refactoring. Several of the tests open ActionController::Helpers#master_helper_module to add helper methods. But that class has moved to AbstractController:Helpers, the method has changed to _helpers, and now it feels like there's an encapsulation boundary there, and opening it up is the wrong way to go about it.

  Working on a revised patch.

• You flagged this item as spam.
  

  Erik Ostrom September 27th, 2009 @ 02:26 AM

  Never mind - it's over my head. Attached is a revision of Elad's patch that got me past the "tests don't run at all" phase and into the "dozens of test failures" phase. This might or might not be better than starting from Elad's patch, or from the original commit.

  • 0001-lh3260-ActionView-Testcase-ported-to-master.patch 14.5 KB

• You flagged this item as spam.
  

  Blue Box Stephen September 27th, 2009 @ 07:11 AM

  -1, confirming Erik's assessment that Elad's patch doesn't seem to work against master. And after several hours of basically trying to duplicate what Erik was doing, I think this is also beyond me.

• 

  Eloy Duran September 27th, 2009 @ 12:49 PM

  Thanks a lot for trying though guys! I'll do this myself then when I find the time.

• You flagged this item as spam.
  

  Erik Ostrom September 27th, 2009 @ 11:14 PM

  Much closer. I expect to have a working patch later today.

• You flagged this item as spam.
  

  Erik Ostrom September 28th, 2009 @ 07:54 AM

  I've attached a patch that makes all the tests pass. I haven't actually verified that you can use it in a real app. But I'm optimistic!

  I am somewhat skeptical about the code around test helpers. It breaks the abstraction boundary around AbstractController::Helpers. It would be better if it could use the Helpers API (or if the Helpers API could be extended for that purpose).

  • 0001-Ported-Eloy-Duran-s-ActionView-TestCase-rewrite-to.patch 15.5 KB
• 

  Eloy Duran September 28th, 2009 @ 09:54 AM

  It looks good, I'll try it out tonight.

  I am somewhat skeptical about the code around test helpers. It breaks the abstraction boundary around AbstractController::Helpers. It would be better if it could use the Helpers API (or if the Helpers API could be extended for that purpose).

  This code breaks the abstraction boundary around AbstractController::Helpers, and in fact includes tests to verify that breaking the abstraction boundary is effective.

  Could you elaborate on this? For instance, in the which tests is “breakage” tested?

  And I assume you're talking about the ActionController::Helpers/AbstractController::Helpers API? That could certainly be refactored yes, for instance, the helpers shouldn't use “controller” imo to call helper methods on. Otherwise, like in this example, it has to be monkeypatched. So one way would be if Helpers.helper_method, accepts both the method to make available, plus the receiver, instead of assuming “controller” as the receiver. The controller class can then simply override it and super with self as the receiver.

• You flagged this item as spam.
  

  Erik Ostrom September 28th, 2009 @ 06:11 PM

  Yeah, that extension of helper_method is just what I had in mind.

  I was confused when I wrote about "tests to verify". I was referring to "is able to make methods available to the view" and "named routes can be used from helper included in view", which both open _helpers (formerly master_helper_module). But they do it as a way to set up the test - not as the behavior to be tested. Sorry I got that wrong.

• 

  Eloy Duran September 28th, 2009 @ 07:08 PM

  • State changed from “new” to “verified”
  • Assigned user changed from “Eloy Duran” to “josh”

  I created an application with a helper, and some simple tests, routes, rendering with ivars and lvars etc. It all worked fine, thanks! So +1 and assigning to Josh. :)

  I just had to add one extra require statement to the test case file. This problem was only encountered when running from an app instead of running the actionpack test suite.

  I created a new patch with the extra require and adjusted the commit message. All ok with you?

• 

  Eloy Duran September 28th, 2009 @ 07:11 PM

  On the topic of the Helpers API etc. Today I thought it would be better to forward almost all method calls to the TestController instance, this would make params, session, request etc work a expected. So these current "hacks" are then probably no longer needed, the only hack that would be needed is to override render so it sets the ivars on the controller and then forwards the call to controller.render which will set the ivars on the view instance.

  Still, if you feel like cleaning what we discussed earlier up right now, feel free of course :)

• 

  Eloy Duran September 28th, 2009 @ 07:11 PM

  Arg, and the patch…

  • ported_actionview_testcase_to_master.patch 15.5 KB
• 

  Repository September 28th, 2009 @ 07:31 PM

  • State changed from “verified” to “resolved”

  (from [8ffc2e3b8de38c485fa24820208b40920dad7ae3]) Ported the new ActionView::TestCase from 2-3-stable to master [#3260 state:resolved]

  The test case now mimicks the template environment more closely, so it's
  possible to use render, load helper dependencies.

  This also fixes assert_select, and similar assertions. Because view tests
  and helpers generally don't render full templates assert_select looks
  first in rendered and then in output_buffer to find the rendered output.

  Additional master'-only changes: Made the Action Pack Rakefile run the<br/> ActionView::TestCase tests, and made ActionView::Rendering#_render_text<br/> always return a string.

  Signed-off-by: Joshua Peek josh@joshpeek.com
  http://github.com/rails/rails/commit/8ffc2e3b8de38c485fa24820208b40...

• 

  Jeremy Kemper October 15th, 2010 @ 11:01 PM

  • Milestone set to “3.0.2”
  • Importance changed from “” to “”

  [bulk edit]