Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Bugfix/feed service tests #743

Merged
merged 2 commits into from

3 participants

@wilkie
Owner

The original tests weren't... um... doing much. :) I mean... look at:

https://github.com/hotsh/rstat.us/blob/master/test/services/feed_service_test.rb#L62-L64

Stubs out every method, asserts that it calls the stub and gets the value stubbed. That pretty much tests ruby's precedence order for the || operator. Apparently, there was this urge in the original developer to stub out private methods. But since FeedService is entirely private methoded, that's not ideal. For instance: FeedService#find_feed_by_remote_url is never called by a test.

Private methods are forced integrations. They must be invoked to test behavior of the public methods. Otherwise, they will never be covered. I did, however, remove db dependency.

I also snuck in the mocha deprecation fix with a mocha update here because this was one of the files which involved the issue, I believe.

Speed

I'm trying to refactor tests in sight of #706 and here are the benchmarks for test/services/feed_service_test.rb

Original

$ rake test:file[test/services/feed_service_test.rb]
test/services/feed_service_test.rb
Rack::File headers parameter replaces cache_control after Rack 1.5.
Run options: --seed 19878

# Running tests: .....

Finished tests in 0.200287s, 24.9641 tests/s, 24.9641 assertions/s.

After ensuring internal codepaths are stubbed

$ rake test:file[test/services/feed_service_test.rb]
test/services/feed_service_test.rb
Rack::File headers parameter replaces cache_control after Rack 1.5.
Run options: --seed 31669

# Running tests: .....

Finished tests in 0.195127s, 25.6243 tests/s, 25.6243 assertions/s.

Stubs out db calls, external codepaths

$ rake test:file[test/services/feed_service_test.rb]
test/services/feed_service_test.rb
Rack::File headers parameter replaces cache_control after Rack 1.5.
Run options: --seed 24722

# Running tests: .....

Finished tests in 0.202075s, 24.7433 tests/s, 29.6920 assertions/s.

I didn't run them on the same seed, but still, no change! These tests are as simple as they can be. I have entire projects with around 200 assertions that run faster than these 5 tests. There is apparently a high upfront overhead of, apparently, 0.2s.

@wilkie
Owner

Removing TestHelper

$ rake test:file[test/services/feed_service_test.rb]
test/services/feed_service_test.rb
Rack::File headers parameter replaces cache_control after Rack 1.5.
Run options: --seed 41627

# Running tests: .....

Finished tests in 0.015902s, 314.4225 tests/s, 377.3070 assertions/s.

Well. This is starting to make a lot of sense. Let's just get rid of our database dependencies in unit tests altogether... should reduce the overhead considerably.

@wilkie
Owner

Removing Require of test_helper.rb

By taking advantage of the lack of requires and going more old school mocking we can remove requiring test_helper altogether:

require "minitest/autorun"
require 'mocha/setup'

class User; end
class Feed; end
class SubscriberToFeedDataConverter; end

require_relative '../../app/services/feed_service'

...

Original refactoring through #743 thus far:

$ time rake test:file[test/services/feed_service_test.rb]
Run options: --seed 45013

Finished tests in 0.015600s, 320.5111 tests/s, 384.6133 assertions/s.

real    0m18.919s
user    0m18.087s
sys     0m0.780s

Not requiring test_helper.rb along with the rest of #743:

$ time rake test:file[test/services/feed_service_test.rb]
Run options: --seed 55063

Finished tests in 0.018919s, 264.2831 tests/s, 317.1397 assertions/s.

real    0m7.612s
user    0m7.283s
sys     0m0.310s

Conclusion: requiring test_helper.rb adds considerable external overhead: ~11s.

@steveklabnik

This needs a rebase after I merged #745.

wilkie added some commits
@wilkie wilkie Fixes FeedService tests to actually test things.
The original tests stubbed the private methods which means they mostly
just tested ruby's precedence order. :) Private methods are forced
integrations... we have to allow them to operate in turn in order to
assess behavior.
fdd12c2
@wilkie wilkie Removing TestHelper from FeedService tests.
This removes the overhead imprint.

Tests run at quick speed:

$ rake test:file[test/services/feed_service_test.rb]
test/services/feed_service_test.rb
Rack::File headers parameter replaces cache_control after Rack 1.5.
Run options: --seed 41627

.....

Finished tests in 0.015902s, 314.4225 tests/s, 377.3070 assertions/s.
b08437d
@carols10cents carols10cents merged commit b08437d into hotsh:master
@carols10cents

looks good to me! :+1:

sorry for letting this sit out here in merge purgatory for so long :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 2, 2013
  1. @wilkie

    Fixes FeedService tests to actually test things.

    wilkie authored
    The original tests stubbed the private methods which means they mostly
    just tested ruby's precedence order. :) Private methods are forced
    integrations... we have to allow them to operate in turn in order to
    assess behavior.
  2. @wilkie

    Removing TestHelper from FeedService tests.

    wilkie authored
    This removes the overhead imprint.
    
    Tests run at quick speed:
    
    $ rake test:file[test/services/feed_service_test.rb]
    test/services/feed_service_test.rb
    Rack::File headers parameter replaces cache_control after Rack 1.5.
    Run options: --seed 41627
    
    .....
    
    Finished tests in 0.015902s, 314.4225 tests/s, 377.3070 assertions/s.
This page is out of date. Refresh to see the latest.
Showing with 29 additions and 5 deletions.
  1. +29 −5 test/services/feed_service_test.rb
View
34 test/services/feed_service_test.rb
@@ -3,8 +3,6 @@
require 'mocha/setup'
describe FeedService do
- include TestHelper
-
describe "#find_or_create!" do
let(:service) { FeedService.new(target_feed) }
let(:existing_feed) { mock }
@@ -15,7 +13,9 @@
let(:target_feed) { "505cc1beb4f2cd000200022c" }
before do
- service.stubs(:find_feed_by_id).returns(existing_feed)
+ Feed.stubs(:first)
+ .with(has_entry(:id, target_feed))
+ .returns(existing_feed)
end
it "returns the feed" do
@@ -29,12 +29,18 @@
let(:user) { mock }
let(:author) { mock }
let(:feed) { mock }
+ let(:uri) { mock }
describe "when the feed exists" do
before do
User.stubs(:find_by_case_insensitive_username).returns(user)
user.stubs(:author).returns(author)
+ Kernel.stubs(:URI).returns(uri)
+ uri.stubs(:host).returns("example.com")
author.stubs(:feed).returns(feed)
+
+ service.stubs(:find_feed_by_id).returns(nil)
+ service.stubs(:find_feed_by_remote_url).returns nil
end
it "returns the feed" do
@@ -45,6 +51,10 @@
describe "when the feed does not exist" do
before do
User.stubs(:find_by_case_insensitive_username).returns(nil)
+ Kernel.stubs(:URI).returns(uri)
+ uri.stubs(:host).returns("example.com")
+
+ service.stubs(:find_feed_by_id).returns(nil)
service.stubs(:find_feed_by_remote_url).returns(existing_feed)
end
@@ -57,11 +67,19 @@
describe "when the feed can by found by remote URL" do
# the remote URL of user 'gavinlaking@rstat.us' (follow me!)
let(:target_feed) { "https://rstat.us/feeds/505cc1beb4f2cd000200022c.atom" }
+ let(:feed_data_cov) { mock }
+ let(:feed_data) { mock }
before do
+ Feed.stubs(:first)
+ .with(has_entry(:remote_url, "feed_url"))
+ .returns(existing_feed)
+ SubscriberToFeedDataConverter.stubs(:new).returns(feed_data_cov)
+ feed_data_cov.stubs(:get_feed_data!).returns(feed_data)
+ feed_data.stubs(:url).returns("feed_url")
+
service.stubs(:find_feed_by_id).returns nil
service.stubs(:find_feed_by_username).returns nil
- service.stubs(:find_feed_by_remote_url).returns(existing_feed)
end
it "returns the feed" do
@@ -71,15 +89,21 @@
describe "when the feed doesn't exist" do
let(:target_feed) { "gavinlaking@rstat.us" } # (follow me!)
+ let(:feed_data_cov) { mock }
+ let(:feed_data) { mock }
before do
service.stubs(:find_feed_by_id).returns nil
service.stubs(:find_feed_by_username).returns nil
service.stubs(:find_feed_by_remote_url).returns nil
- service.stubs(:create_feed_from_feed_data).returns(new_feed)
+ SubscriberToFeedDataConverter.stubs(:new).returns(feed_data_cov)
+ feed_data_cov.stubs(:get_feed_data!).returns(feed_data)
end
it "creates the feed" do
+ Feed.expects(:create_and_populate!)
+ .with(feed_data)
+ .returns(new_feed)
service.find_or_create!.must_equal new_feed
end
end
Something went wrong with that request. Please try again.