Improve unit test coverage for models #1131

pozorvlak · Dec 19, 2016

Adds some unit tests for posts and crops, and does some slight cleanup of the existing tests there. Also adds one feature test for "sign-in using email address rather than username", because that works by implementing a callback for Devise to use, and it didn't seem that unit-testing the callback was very worthwhile: the callback is trivial, the question is whether it matches Devise's expectations.

The two models with big coverage gaps are Member and Photo, because of the code to talk to Gibbon and Flickr. We could (and perhaps should) get those coverage numbers up with some judicious mocking. Edit: I've made a start at this.

The directory with the worst coverage is hands-down app/controllers, some of whose members have dire coverage. We deprecated controller tests a while back in favour of feature tests; in view of our problems with flaky CI, I think we should revisit that decision.

maco · Dec 19, 2016

+1 on revisiting that decision. It also seems like having that much logic in some of the controllers might mean there's some too-tightly-coupled stuff going on.

…

coveralls · Dec 19, 2016

Coverage increased (+0.2%) to 88.648% when pulling abb9acd on pozorvlak:moar_tests into 1b9a5bc on Growstuff:dev.

coveralls · Dec 19, 2016

Coverage increased (+0.3%) to 88.723% when pulling b8385af on pozorvlak:moar_tests into 1b9a5bc on Growstuff:dev.

coveralls · Dec 19, 2016

Coverage increased (+0.3%) to 88.768% when pulling 70f589d on pozorvlak:moar_tests into 1b9a5bc on Growstuff:dev.

coveralls · Dec 19, 2016

Coverage increased (+0.3%) to 88.768% when pulling 70f589d on pozorvlak:moar_tests into 1b9a5bc on Growstuff:dev.

coveralls · Dec 19, 2016

Coverage increased (+0.3%) to 88.768% when pulling 4f1c94b on pozorvlak:moar_tests into 1b9a5bc on Growstuff:dev.

coveralls · Dec 19, 2016

Coverage increased (+0.6%) to 88.985% when pulling 4b1cdc5 on pozorvlak:moar_tests into 1b9a5bc on Growstuff:dev.

Br3nda · Dec 19, 2016

This is a fabulous PR. TEST ALL THE THINGS

CloCkWeRX · Dec 20, 2016

CloCkWeRX reviewed

Dec 20, 2016

View changes

Is it better to consider shifting this to an 'action' or similar pattern later?

class NewsletterAction
  def initialize(gb = ..., testing = ...)

  def subscribe(member)
  def unsubscribe(member)
end

Could you use a double in the specification instead of changing the method?

@Br3nda: I changed the method so I could inject a double :-) I looked at using any_instance, but saw it was deprecated; OTOH, I ended up using receive_message_chain, which is also deprecated. Should I just go ahead and use any_instance, or can you suggest a better way?

CloCkWeRX · Dec 20, 2016

CloCkWeRX reviewed

Dec 20, 2016

View changes

Should we look at VCR or a fake API driver instead of return true at a later point

Yep, that sounds like a good idea.

CloCkWeRX · Dec 20, 2016

CloCkWeRX reviewed

Dec 20, 2016

View changes

I'm not always sold on let! sharing state across tests, I keep getting bitten in subtle ways I didn't expect - I tend to prefer a context and before to describe the test setup; then do expectations about the results

CloCkWeRX · Dec 20, 2016

CloCkWeRX approved these changes

Dec 20, 2016

View changes

LGTM overall

CloCkWeRX · Dec 20, 2016

We seem to have a lot of scope/sql tests, which means we're doing FactoryGirl.create a lot to set up database state.

Maybe down the track we can tweak stuff like

def Member.interesting
    howmany = 12 # max number to find
    interesting_members = []
    Member.confirmed.located.recently_signed_in.each do |m|
      break if interesting_members.size == howmany
      if m.interesting?
        interesting_members.push(m)
      end
    end
    interesting_members
  end

into two parts: "Fetch data from store/wherever" and "manipulate data to results we want"; and only really test the 'manipulate' side of things with model specs.

Right now that above method could be almost entirely stuffed into the SQL layer as something like:

SELECT members.* 
FROM members 

JOIN plantings ON members.id = plantings.member_id # This is currently 'interesting?' - has a planting

WHERE
   confirmed_at IS NOT NULL
AND
  location <> '' and latitude IS NOT NULL and longitude IS NOT NULL
GROUP BY members.id
ORDER BY confirmed_at DESC LIMIT 12

Br3nda · Dec 20, 2016

Br3nda approved these changes

Dec 20, 2016

View changes

pozorvlak · Dec 20, 2016

Merging this since it has two +1s. This approach to testing the Gibbon-related code isn't ideal, but we can improve it when we add test coverage for the Flickr code, which will have similar issues.

maco added the in progress label Dec 19, 2016

pozorvlak merged commit cf31e53 into Growstuff:dev Dec 20, 2016
2 checks passed

2 checks passed

Details codeclimate no new or fixed issues

Details continuous-integration/travis-ci/pr The Travis CI build passed

maco removed the in progress label Dec 20, 2016

pozorvlak deleted the pozorvlak:moar_tests branch Dec 20, 2016

cesy modified the milestone: Release 17 Dec 21, 2016

Miles Gould	`Test Crop.create_from_csv with missing parent` I couldn't work out how to test that the error is logged.		`b40974d`
Miles Gould	`Test Post.comment_count`		`b011fc9`
Miles Gould	`Test Post.recently_active`		`c23e80b`
Miles Gould	`Test [member name](member) mention syntax`		`7239cf0`
Miles Gould	`Replace << with string-interpolation in post_spec`		`59b86c9`
Miles Gould	`post_spec: replace instance vars with local vars` The @ in front of a variable makes it an instance variable of the surrounding object. There's no point doing that for variables that are only used in a single function, so I've made such variables in the file post_spec.rb into function-local variables. We should do this more generally: there are a lot of instances of this (anti?)pattern in our test suite.		`67faa55`
Miles Gould	`post_spec: fix warnings` Several "unused variable" warnings (due to replacing unused instance variables with unused locals) and one warning about ambiguous lack-of-brackets.	All checks have passed 2 successful checks Details codeclimate — no new or fixed issues Details continuous-integration/travis-ci/pr — The Travis CI build passed	`abb9acd`

Miles Gould	`Feature test for signin via email address`		`3469d6d`
Miles Gould	`Improve tests for Member.interesting`		`f5eede6`
Miles Gould	`Replace instance vars with locals in member_spec`	Some checks were not successful 1 failing and 1 successful checks Details continuous-integration/travis-ci/pr — The Travis CI build failed Details codeclimate — no new or fixed issues	`70f589d`

@@ -239,9 +239,8 @@ def update_newsletter_subscription
          end
        end
-    -  def newsletter_subscribe(testing = false)
+    +  def newsletter_subscribe(gb = Gibbon::API.new, testing = false)

@@ -250,9 +249,8 @@ def newsletter_subscribe(testing = false)
                             })
        end
-    -  def newsletter_unsubscribe(testing = false)
+    +  def newsletter_unsubscribe(gb = Gibbon::API.new, testing = false)
          return true if (Rails.env.test? && !testing)

@@ -76,7 +76,7 @@
            Time.stub(now: Time.now)
          end
-    -    let(:post) { FactoryGirl.create(:post, created_at: 1.day.ago) }
+    +    let!(:post) { FactoryGirl.create(:post, created_at: 1.day.ago) }

Growstuff/growstuff

Improve unit test coverage for models #1131

Reviewers

Assignees

Labels

Projects

Milestone

6 participants

2 checks passed

Growstuff/growstuff

Improve unit test coverage for models #1131

Reviewers

Assignees

Labels

Projects

Milestone

6 participants

All checks have passed

Some checks were not successful

Some checks were not successful

All checks have passed

All checks have passed

2 checks passed