Use rspec-mock instead of rr #3552

Merged
merged 1 commit into from Mar 7, 2015

Conversation

Projects
None yet
4 participants
@arthurnn
Contributor

arthurnn commented Mar 7, 2015

rr gem is not maintained anymore. We can replace it with rspec-mock, and stop using mocking in a few spots.

review @parkr @envygeeks
[fixes #3549]

@@ -12,7 +12,7 @@ class TestCommand < JekyllUnitTest
context "when fatal error occurs" do
should "exit with non-zero error code" do
site = Object.new
- stub(site).process { raise Jekyll::Errors::FatalException }
+ def site.process; raise Jekyll::Errors::FatalException; end

This comment has been minimized.

@parkr

parkr Mar 7, 2015

Member

?!

This comment has been minimized.

@arthurnn

arthurnn Mar 7, 2015

Contributor

no need to stub here, you can just re-define the method and make it raise.

@arthurnn

arthurnn Mar 7, 2015

Contributor

no need to stub here, you can just re-define the method and make it raise.

This comment has been minimized.

@arthurnn

arthurnn Mar 7, 2015

Contributor

btw, we do that a lot on Rails test suite.

@arthurnn

arthurnn Mar 7, 2015

Contributor

btw, we do that a lot on Rails test suite.

This comment has been minimized.

@parkr

parkr Mar 7, 2015

Member

is this reset later?

@parkr

parkr Mar 7, 2015

Member

is this reset later?

This comment has been minimized.

@arthurnn

arthurnn Mar 7, 2015

Contributor

actually that method will be only defined to that instance. site is not shared with other tests so it is fine.

@arthurnn

arthurnn Mar 7, 2015

Contributor

actually that method will be only defined to that instance. site is not shared with other tests so it is fine.

This comment has been minimized.

@parkr

parkr Mar 7, 2015

Member

This is totally new to me, very awesome!!

@parkr

parkr Mar 7, 2015

Member

This is totally new to me, very awesome!!

test/test_related_posts.rb
'destination' => dest_dir})
- end
- @site = Site.new(Jekyll.configuration)
+ @site = Site.new(config)

This comment has been minimized.

@parkr

parkr Mar 7, 2015

Member

This can all be simplified down to:

@site = fixture_site

it uses source_dir and dest_dir by default.

@parkr

parkr Mar 7, 2015

Member

This can all be simplified down to:

@site = fixture_site

it uses source_dir and dest_dir by default.

test/test_related_posts.rb
- any_instance_of(Jekyll::RelatedPosts) { |i| stub(i).display }
- @site = Site.new(Jekyll.configuration)
+ allow_any_instance_of(Jekyll::RelatedPosts).to receive(:display)
+ @site = Site.new(config)

This comment has been minimized.

@parkr

parkr Mar 7, 2015

Member

Then this can be

@site = fixture_site({"lsi": => true})

No config step needed.

@parkr

parkr Mar 7, 2015

Member

Then this can be

@site = fixture_site({"lsi": => true})

No config step needed.

- }.merge(override))
- end
- site = Site.new(Jekyll.configuration)
+ site = fixture_site({"highlighter" => "rouge"}.merge(override))

This comment has been minimized.

@parkr

parkr Mar 7, 2015

Member

this won't deep-merge, not sure if it's needed

@parkr

parkr Mar 7, 2015

Member

this won't deep-merge, not sure if it's needed

This comment has been minimized.

@arthurnn

arthurnn Mar 7, 2015

Contributor

I just kept the way it was before.
So you are saying we dont need the {"highlighter" => "rouge"} part?

@arthurnn

arthurnn Mar 7, 2015

Contributor

I just kept the way it was before.
So you are saying we dont need the {"highlighter" => "rouge"} part?

This comment has been minimized.

@parkr

parkr Mar 7, 2015

Member

i'm an idiot, there's nothing to deep-merge :) don't mind me...

@parkr

parkr Mar 7, 2015

Member

i'm an idiot, there's nothing to deep-merge :) don't mind me...

test/test_tags.rb
- site = Site.new(Jekyll.configuration)
+ site = Site.new(configuration)

This comment has been minimized.

@parkr

parkr Mar 7, 2015

Member

let's use fixture_site here as well :)

@parkr

parkr Mar 7, 2015

Member

let's use fixture_site here as well :)

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 7, 2015

Member

Thank you, @arthurnn!!!!!!

Member

parkr commented Mar 7, 2015

Thank you, @arthurnn!!!!!!

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Mar 7, 2015

Contributor

You are welcome.. thanks for Jekyll !
🎉 everything is 💚 now.

Contributor

arthurnn commented Mar 7, 2015

You are welcome.. thanks for Jekyll !
🎉 everything is 💚 now.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 7, 2015

Member

LGTM! :shipit:

Member

parkr commented Mar 7, 2015

LGTM! :shipit:

parkr added a commit that referenced this pull request Mar 7, 2015

Merge pull request #3552 from arthurnn/rm_rr
Use rspec-mock instead of rr

@parkr parkr merged commit 9021e98 into jekyll:master Mar 7, 2015

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

parkr added a commit that referenced this pull request Mar 7, 2015

@arthurnn arthurnn deleted the arthurnn:rm_rr branch Mar 7, 2015

parkr added a commit that referenced this pull request Mar 8, 2015

Merge branch 'master' of github.com:jekyll/jekyll
* 'master' of github.com:jekyll/jekyll:
  Update history to reflect merge of #3552 [ci skip]
  Use rspec-mock instead of rr
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 13, 2015

Contributor

Sorry for saying thanks late! ❤️

Contributor

envygeeks commented Mar 13, 2015

Sorry for saying thanks late! ❤️

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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