5

I have a private method in a controller

private 
  def body_builder
    review_queue = ReviewQueueApplication.where(id: params[:review_queue_id]).first
    ...
    ...
  end

I would like to test just the body_builder method, it is a method buidling the payload for an rest client api call. It needs access to the params however.

describe ReviewQueueApplicationsController, type: :controller do
  describe "when calling the post_review action" do
    it "should have the correct payload setup" do
      @review_queue_application = ReviewQueueApplication.create!(application_id: 1)
      params = ActionController::Parameters.new({ review_queue_id: @review_queue_application.id })
      expect(controller.send(:body_builder)).to eq(nil)
    end
  end
end

If I run the above it will send the body_builder method but then it will break because the params have not been set up correctly as they would be in a call to the action.

I could always create a conditional parameter for the body_builder method so that it either takes an argument or it will use the params like this def body_builder(review_queue_id = params[:review_queue_id]) and then in the test controller.send(:body_builder, params), but I feel that changing the code to make the test pass is wrong it should just test it as it is.

How can I get params into the controller before I send the private method to it?

legendary_rob
  • 12,792
  • 11
  • 56
  • 102
  • My suggestion would be to actually CALL the RESTful entry point with the correct params. If you've got behavior that has to occur before your body_builder method can be called, you can mock and stub to channel the execution down the right path. Then you place an expectation on the controller.body_builder method rather than just calling it. I realize this is heavier than just calling the private method, but I've always felt that if one calls private private methods directly in tests you're kind of stepping outside the test "sandbox" for that object. – jaydel May 05 '16 at 13:33
  • @jaydel, I totally get where you are coming from, the only issue is in testing the actual call to the entry point is that it will fire off the `RestClient::Request` action which is already tested, I want to assume that that portion works correctly but the building of the payload is neglected, if we add things to the payload that it is not expecting it should fail. – legendary_rob May 05 '16 at 13:48
  • 1
    Yes, I understand your concern. I'm unfamiliar with the details of that particular behavior, but is it possible to mock and stub that part to build the payload in the ways you want to test. I've wandered outside the context I understand though, and you know it much better than I do, of course. so just food for thought – jaydel May 05 '16 at 13:56
  • This is a great question you've asked. I have lived through a lot of contentious conversations about this issue. For whatever reason there seems to be some 'religion' around this topic. I tend to avoid taking those positions blindly and am still unresolved in many ways, so I'll be following this question :) – jaydel May 05 '16 at 13:57
  • Yeah, I feel the same way, we break up the code in the controllers all the time like this and it make's sense to be able to test each method according to its responsibility. But having a global state like parameters make it tough to test them on their own. You have to constantly be aware of that rails black magic in the background. – legendary_rob May 05 '16 at 14:02
  • a temporary fix would be to add `controller.params = params` in the test just before the expect. This hack's the params into the test instance of the controller. seems pretty dirty – legendary_rob May 05 '16 at 14:04
  • I wouldn't call that dirty. You are setting up a complex object, a controller instance, and part of that is setting its attributes, which is what the params is. – niels May 05 '16 at 14:08
  • Thinking more about it, if there's a lot happening in a controller action that needs to be tested, that could signal that domain logic is leaking into the the controller, which perhaps could be split off in a separate class, which wouldn't need to rely on `params`, but just on a parameter being passed. – niels May 05 '16 at 14:26

1 Answers1

3

I think you should be able to replace 

params = ActionController::Parameters.new({ review_queue_id: @review_queue_application.id })

with

controller.params = ActionController::Parameters.new({ review_queue_id: @review_queue_application.id })

and you should be good. The params is just an attribute of the controller (the actual attribute is @_params but there are methods for accessing that ivar. Try putting controller.inspect in a view).

niels
  • 1,719
  • 10
  • 20
  • by the way, having said this, you shouldn't be testing private methods, of course ;-) – niels May 05 '16 at 14:04
  • 1
    shouldn't be testing them ***directly***, perhaps? If they're complicated and have logic that can go wrong, they should be tested in my opinion. – jaydel May 05 '16 at 14:06
  • I have a little policy that is if I get an exception: I immediately start the process by writing a test. This way the I know I will never get an exception, most of my private methods are carrying heavy IP requirements and break quite a bit. Even though it seems dirty by sending these methods off, my thinking is if it's really hard to test private methods then something is wrong with the class I am writing. – legendary_rob May 05 '16 at 14:09
  • I believe the general opinion on this is that private methods should only be tested through public methods. So if your private method is causing exceptions, I would normally test that through the public method that uses the private method. – niels May 05 '16 at 14:12
  • 1
    yes, that's what I mean by directly. You can test them specifically through isolation with mocking and stubbing and use public interfaces to drive those tests. my assertion (and it's just my opinion) is that a) they should be tested and b) your tests should not use `.send` to test them. – jaydel May 05 '16 at 14:14
  • Agree! For some discussion of testing private methods: http://stackoverflow.com/questions/105007/should-i-test-private-methods-or-only-public-ones – niels May 05 '16 at 14:16