Add Factorygirl as an alternative to fixtures #1279

Merged
merged 5 commits into from Sep 8, 2016

Conversation

Projects
None yet
2 participants
@gravitystorm
Collaborator

gravitystorm commented Sep 7, 2016

FactoryGirl is a widely used alternative to fixtures in tests. The fixtures are annoying since every time you want to test something slightly different (for example, a diary entry with an apostrophe in its title), you need to add a whole new fixture and rework the tests. Fixtures are also a pain when the model attributes change.

This PR adds FactoryGirl, and reworks two model tests (DiaryEntry and DiaryComment) to use them.

gravitystorm added some commits Sep 7, 2016

Rework DiaryEntry and DiaryComment model tests to use factories.
Since the database also contains fixtures from other tests, some
counts are dropped and instead tested for inclusion in the results.
test/models/diary_comment_test.rb
@@ -1,10 +1,8 @@
require "test_helper"
class DiaryCommentTest < ActiveSupport::TestCase
- api_fixtures
- fixtures :diary_comments
-
def test_diary_comment_count

This comment has been minimized.

@tomhughes

tomhughes Sep 7, 2016

Member

This test name doesn't really reflect what the test does any more... Then again I'm not really sure what it is testing now - is it actually just testing that FactoryGirl can create records? or does that itself funnel through the rails code so that we're testing rails can create records?

@tomhughes

tomhughes Sep 7, 2016

Member

This test name doesn't really reflect what the test does any more... Then again I'm not really sure what it is testing now - is it actually just testing that FactoryGirl can create records? or does that itself funnel through the rails code so that we're testing rails can create records?

test/models/diary_entry_test.rb
- assert_equal 0, diary_entries(:normal_user_entry_1).comments.count
- assert_equal 4, diary_entries(:normal_user_geo_entry).comments.count
+ diary = create(:diary_entry)
+ create(:diary_comment, :diary_entry => diary)

This comment has been minimized.

@tomhughes

tomhughes Sep 7, 2016

Member

I wonder if a more canonical way to do this is something like:

diary = create(:diary_entry) do |diary|
   diary.comments.create(attributes_for(:diary_comment))
end

I mean I don't really know if it's any better or worse, it's just the sort of thing I was seeing in the FactoryGirl docs.

@tomhughes

tomhughes Sep 7, 2016

Member

I wonder if a more canonical way to do this is something like:

diary = create(:diary_entry) do |diary|
   diary.comments.create(attributes_for(:diary_comment))
end

I mean I don't really know if it's any better or worse, it's just the sort of thing I was seeing in the FactoryGirl docs.

This comment has been minimized.

@tomhughes

tomhughes Sep 7, 2016

Member

Then again maybe we should leave it as is but add an assertion that there are zero comments before that create...

@tomhughes

tomhughes Sep 7, 2016

Member

Then again maybe we should leave it as is but add an assertion that there are zero comments before that create...

test/models/diary_entry_test.rb
end
private
def diary_entry_valid(attrs, result = true)
- entry = DiaryEntry.new(diary_entries(:normal_user_entry_1).attributes)
+ entry = DiaryEntry.new(attributes_for(:diary_entry))

This comment has been minimized.

@tomhughes

tomhughes Sep 7, 2016

Member

I this not just entry = build(:diary_entry)?

@tomhughes

tomhughes Sep 7, 2016

Member

I this not just entry = build(:diary_entry)?

This comment has been minimized.

@tomhughes

tomhughes Sep 7, 2016

Member

Indeed can we not merge it with the next line as:

entry = build(:diary_entry, attrs)
@tomhughes

tomhughes Sep 7, 2016

Member

Indeed can we not merge it with the next line as:

entry = build(:diary_entry, attrs)
@gravitystorm

This comment has been minimized.

Show comment
Hide comment
@gravitystorm

gravitystorm Sep 8, 2016

Collaborator

Thanks for the comments. One of the problems is that the original tests are a bit pointless - they are mostly asserting that the fixtures have loaded, or that Rails itself is working (e.g. build a model from attributes, whatdoyouknow, the model has those attributes). The refactored versions are still mostly trivial, and often a bit pointless too.

I would prefer to rewrite most of these tests completely to test our "business logic" instead. But I was wary of trying to do too many things at once, since I'm always suspicious when people start rewriting tests instead of refactoring them!

I'll fix the specific points you've raised, and make a separate PR with some more thorough rewriting.

Collaborator

gravitystorm commented Sep 8, 2016

Thanks for the comments. One of the problems is that the original tests are a bit pointless - they are mostly asserting that the fixtures have loaded, or that Rails itself is working (e.g. build a model from attributes, whatdoyouknow, the model has those attributes). The refactored versions are still mostly trivial, and often a bit pointless too.

I would prefer to rewrite most of these tests completely to test our "business logic" instead. But I was wary of trying to do too many things at once, since I'm always suspicious when people start rewriting tests instead of refactoring them!

I'll fix the specific points you've raised, and make a separate PR with some more thorough rewriting.

@tomhughes

This comment has been minimized.

Show comment
Hide comment
@tomhughes

tomhughes Sep 8, 2016

Member

Yes I always find that a lot of model tests involve a fair bit of "what exactly are we really testing here" head scratching...

I mean obviously if you have a method with some additional logic in it on a model that's fine and can be tested but a lot of the time the tests as you say seem to be testing rails more than our code. I think things that make sense in a model test are:

  • That any additional methods written by us work
  • That associations return the expected records, especially where they involved filtering, ordering, etc
  • That validations allow/reject the correct things

On the other handing, testing that count returns the right number of records is probably less useful ;-)

Member

tomhughes commented Sep 8, 2016

Yes I always find that a lot of model tests involve a fair bit of "what exactly are we really testing here" head scratching...

I mean obviously if you have a method with some additional logic in it on a model that's fine and can be tested but a lot of the time the tests as you say seem to be testing rails more than our code. I think things that make sense in a model test are:

  • That any additional methods written by us work
  • That associations return the expected records, especially where they involved filtering, ordering, etc
  • That validations allow/reject the correct things

On the other handing, testing that count returns the right number of records is probably less useful ;-)

@tomhughes tomhughes merged commit 191f6b3 into openstreetmap:master Sep 8, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.07%) to 89.75%
Details

@gravitystorm gravitystorm deleted the gravitystorm:factorygirl branch Oct 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment