Test index action with rspec and factory girl

Testing
1

I’m trying to write a test for a legacy app that uses Rspec and FactoryGirl but has a lot of test gaps, and unfortunately, I’m not all that strong in either Rspec or FG.

So, what I’m trying to test is a controller index action, that takes a few different parameters to filter the results.

I’ve also tried using the let(…) syntax to setup the initial data, but in no matter what I do, I seem to get errors where things are nil when they shouldn’t be. (eg, undefined method `chr’ for nil:NilClass). Obviously, I’m not understanding something fundamental about either Rspec or FactoryGirl. Any assistance in getting me on the right track here would be much appreciated.

Thanks.

https://gist.github.com/jsgarvin/dc8365e7405aa2eb5b5a

2

Hi Jonathan! The formatting on your question seems to be a bit off near the end. Can you edit it so that the text doesn’t run into itself? Not sure if there is a missing tag or if code blocks are limited to a certain length. If the length is an issue, including the set up and two tests should be enough for you to get some good feedback!

Including the actual controller method would be helpful, too!

3

I don’t know what the problem with the formatting is. Looks fine in the preview window. I’ve tried making some changes to fix it, but disqus doesn’t seem to like preformatted blocks that are that long.

4

Hi @jsgarvin,

You can create a private gist (which Github calls “secret gists”) here: http://gist.github.com, and paste the controller and the spec in the same post, that might make review easier.

5

Good idea @geoffharcourt. I replaced the code block in the OP with a gist. Formatting is still wonky, but at least now you can hopefully click through to the gist itself.

I still left out the get_scenarios() method that the controller calls, as it’s pretty long and not really the focus of my question. My concern is that, when doing this type of test, I feel like I must be approaching this completely wrong from a Rspec/FactoryGirl best practices standpoint, and that’s why I’m running into issues.

6

Also, I’m not in any position at this point to ‘fix’ any bad coding in the controller (as much as I would like to). It works in production. I’m just trying to get some test coverage for it. Refactoring the controller will come later.

7

updated the gist with the current version of my describe block. With this version everything passes except line 57. That line throws the following error…

Failure/Error: expect(assigns(:scenarios)).to include(@test_scenario)
NoMethodError:
   undefined method `chr' for nil:NilClass
8

ahhh, damn. I needed to wrap the “true” for that “get” in quotes. Now every thing passes. (sigh). Ok, I’m still very interested if anyone has any input on a more appropriate / best practices way to approach testing this kind of controller action.

9

Hi Jonathan! Thanks for pasting in that gist. And awesome you were able to find the solution yourself! I don’t do a lot of controller testing (mostly unit tests and integration tests using capybara and capybara-webkit) but here are a few ideas for how to improve the tests above:

  1. Mix the FactoryGirl syntax methods in by including the following in your spec_helper.rb file:

       RSpec.configure do |config|
     config.include FactoryGirl::Syntax::Methods
   end

    That way, you can remove all of the instance of ‘FactoryGirl.’ in this file and all other spec files.

  2. Clean up naming

It wasn’t clear to me why the following expectations are expected in your first spec:

    expect(assigns(:scenarios)).to_not include(@test_scenario)
    expect(assigns(:scenarios)).to_not include(@another_test_scenario)

This isn’t necessarily because you have a bad test. It is more likely that the naming of "test scenario’ and “another_test_scenario” is not revealing why it should be be included. When looking at the setup, I see that the scenario is using a factory called ‘test_scenario.’ How is this factory different from a real scenario? Does your project have a model called test scenario or is this factory creating a scenario with different traits? If the latter is the case, I would consider taking advantage of FactoryGirl’s traits: Remove Duplication with FactoryBot's Traits

  1. Add setup within each spec

For the test with the describe block of

    `it 'returns real scenarios for a specific user' do

it is a little confusing that the expectations are the similar to the expectations of your first spec that does not include a user. When looking at the setup, it makes more sense. This is why I generally prefer to include my setup within each test. If you did that, you could create one user and two scenarios for that particular test, one named user_scenario and the other named other_user_scenario, which would belong to another user, and include the following expectations:

    expect(assigns(:scenarios)).to include(user_scenario)
    expect(assigns(:scenarios)).to_not include(other_user_scenario)

If you went with this method (including setup within each spec and naming factory objects more explicitly), it would also be helpful to separate setup, exercise, verification, and teardown phases with newlines.

Hope these ideas help you, Jonathan! Have a great day.

Jessie

10

Awesome. Thanks for the tips @jessie. Much appreciated.

1 Like