Rspec 3 #3403

Merged
merged 4 commits into from Apr 27, 2015

Conversation

7 participants
@arthurnn
Contributor

arthurnn commented Apr 25, 2015

Main motivation: rspec 2 is not supported anymore, and some gems dont support rails 4.2 + rspec 2.
Fix: Update rspec to 3.x

I used this fantastic tool http://yujinakayama.me/transpec/ to do the heavy lift. And did a few other necessary manual changes.

review @SamSaffron @chancancode @coding-horror

arthurnn added some commits Apr 25, 2015

Convert specs to RSpec 2.99.2 syntax with Transpec
This conversion is done by Transpec 3.1.0 with the following command:
    transpec

* 424 conversions
    from: obj.should
      to: expect(obj).to

* 325 conversions
    from: == expected
      to: eq(expected)

* 38 conversions
    from: obj.should_not
      to: expect(obj).not_to

* 15 conversions
    from: =~ /pattern/
      to: match(/pattern/)

* 9 conversions
    from: it { should ... }
      to: it { is_expected.to ... }

* 5 conversions
    from: lambda { }.should_not
      to: expect { }.not_to

* 4 conversions
    from: lambda { }.should
      to: expect { }.to

* 2 conversions
    from: -> { }.should
      to: expect { }.to

* 2 conversions
    from: -> { }.should_not
      to: expect { }.not_to

* 1 conversion
    from: === expected
      to: be === expected

* 1 conversion
    from: =~ [1, 2]
      to: match_array([1, 2])

For more details: https://github.com/yujinakayama/transpec#supported-conversions
@discoursebot

This comment has been minimized.

Show comment
Hide comment
@discoursebot

discoursebot Apr 25, 2015

You've signed the CLA, arthurnn. Thank you! This pull request is ready for review.

You've signed the CLA, arthurnn. Thank you! This pull request is ready for review.

+ expect(client.count).to eq(1)
+ expect(client[locale]).not_to eq(nil)
+ expect(client[locale].count).to eq(2)
+ expect(client[locale]["js"]).not_to eq(nil)

This comment has been minimized.

@kirs

kirs Apr 25, 2015

What about changing ``not_to eq(nil)to.to be_present`? I think it's better readable.

@kirs

kirs Apr 25, 2015

What about changing ``not_to eq(nil)to.to be_present`? I think it's better readable.

This comment has been minimized.

@arthurnn

arthurnn Apr 25, 2015

Contributor

good call. will do that.

@arthurnn

arthurnn Apr 25, 2015

Contributor

good call. will do that.

@ZogStriP

This comment has been minimized.

Show comment
Hide comment
@ZogStriP

ZogStriP Apr 25, 2015

Member

That's awesome. Would you mind rebasing and squashing into 1 commit?

Member

ZogStriP commented Apr 25, 2015

That's awesome. Would you mind rebasing and squashing into 1 commit?

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Apr 25, 2015

👍 It's nice to see projects upgrading to RSpec 3 :). Two questions/suggestions:

  • Is there any reason you updated to RSpec 3.1 and not 3.2? 3.2 should be a drop-in replacement for 3.1 as long as you're only using RSpec's public APIs (and given that you were able to upgrade from 2.x to 3.x, I don't think you're using any of our private APIs). 3.3 is also just around the corner...
  • To get the full benefits of RSpec 3 you may want to update your config to enable some new config options in RSpec 3. See .rspec and spec/spec_helper.rb for the recommended config options that 3.1 generates when you use rspec --init on a new project. (If you update to 3.2 you may want to consult those files for v3.2.0).

👍 It's nice to see projects upgrading to RSpec 3 :). Two questions/suggestions:

  • Is there any reason you updated to RSpec 3.1 and not 3.2? 3.2 should be a drop-in replacement for 3.1 as long as you're only using RSpec's public APIs (and given that you were able to upgrade from 2.x to 3.x, I don't think you're using any of our private APIs). 3.3 is also just around the corner...
  • To get the full benefits of RSpec 3 you may want to update your config to enable some new config options in RSpec 3. See .rspec and spec/spec_helper.rb for the recommended config options that 3.1 generates when you use rspec --init on a new project. (If you update to 3.2 you may want to consult those files for v3.2.0).
@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Apr 25, 2015

Contributor

@ZogStriP I didnt want to squash the commits in one, because logically they have different meaning, let me walk you through them:

  • Just ran Transpec command to migrate to new syntax
  • Fixed one migrated test that didnt work
  • Did the Gem upgrade
  • Fixed the warnings

Of course I can squash them in one commit, however the good thing, specially about the first commit, is that it contains all the stats of the changes in the commit message.
@myronmarston I will go and move to 3.2, and apply your suggestions, thanks for reviewing it ❤️

Contributor

arthurnn commented Apr 25, 2015

@ZogStriP I didnt want to squash the commits in one, because logically they have different meaning, let me walk you through them:

  • Just ran Transpec command to migrate to new syntax
  • Fixed one migrated test that didnt work
  • Did the Gem upgrade
  • Fixed the warnings

Of course I can squash them in one commit, however the good thing, specially about the first commit, is that it contains all the stats of the changes in the commit message.
@myronmarston I will go and move to 3.2, and apply your suggestions, thanks for reviewing it ❤️

@myronmarston

View changes

.rspec
@@ -1 +1,2 @@
--colour
+--require spec_helper

This comment has been minimized.

@myronmarston

myronmarston Apr 25, 2015

You actually may not want this here. The point of putting this here (as recommended by the skeleton RSpec 3 generates) is to ensure it's always loaded w/o having to repeat require 'spec_helper' at the top of every spec file. However, it doesn't look like you bothered to remove the require 'spec_helper' lines from the spec files. Also, if you have this line in .rspec, it's important that spec_helper is really light weight since it will always get loaded. The spec_helper on the project is currently loading heavyweight dependencies like rspec-rails, rails, etc -- but presumably, some spec files aren't going to need those things loaded, and having this required is going to slow down running them. That's the reason for this note at the top of the generated spec_helper:

https://github.com/rspec/rspec-core/blob/v3.2.3/lib/rspec/core/project_initializer/spec/spec_helper.rb#L7-L13

rspec-rails 3 follows this principle by keeping spec_helper nice and light weight, with minimal config that you'll always want, and also providing a rails_helper that loads rspec-rails and rails and can be required by the files that need rails loaded.

In the short term, if you want to add config like this, I'd suggest making a new spec/minimal_spec_helper.rb (or spec/fast_spec_helper.rb or whatever), moving the light-weight, non-rails config there (such as mock_framework = :mocha and order = 'random'), and updating this to require that file. Or maybe punt on this for now until the spec_helper code has been split into spec_helper and rails_helper.

@myronmarston

myronmarston Apr 25, 2015

You actually may not want this here. The point of putting this here (as recommended by the skeleton RSpec 3 generates) is to ensure it's always loaded w/o having to repeat require 'spec_helper' at the top of every spec file. However, it doesn't look like you bothered to remove the require 'spec_helper' lines from the spec files. Also, if you have this line in .rspec, it's important that spec_helper is really light weight since it will always get loaded. The spec_helper on the project is currently loading heavyweight dependencies like rspec-rails, rails, etc -- but presumably, some spec files aren't going to need those things loaded, and having this required is going to slow down running them. That's the reason for this note at the top of the generated spec_helper:

https://github.com/rspec/rspec-core/blob/v3.2.3/lib/rspec/core/project_initializer/spec/spec_helper.rb#L7-L13

rspec-rails 3 follows this principle by keeping spec_helper nice and light weight, with minimal config that you'll always want, and also providing a rails_helper that loads rspec-rails and rails and can be required by the files that need rails loaded.

In the short term, if you want to add config like this, I'd suggest making a new spec/minimal_spec_helper.rb (or spec/fast_spec_helper.rb or whatever), moving the light-weight, non-rails config there (such as mock_framework = :mocha and order = 'random'), and updating this to require that file. Or maybe punt on this for now until the spec_helper code has been split into spec_helper and rails_helper.

This comment has been minimized.

@arthurnn

arthurnn Apr 25, 2015

Contributor

@myronmarston thanks a lot for this explanation ❤️ ❤️ ❤️ ❤️ ❤️ , I am really glad you are here to catch my mistakes 😸 .

For now, I will remove this, and let the test files require spec_helper. After that we can try to split that in 2 files as you suggested.
thanks again.

@arthurnn

arthurnn Apr 25, 2015

Contributor

@myronmarston thanks a lot for this explanation ❤️ ❤️ ❤️ ❤️ ❤️ , I am really glad you are here to catch my mistakes 😸 .

For now, I will remove this, and let the test files require spec_helper. After that we can try to split that in 2 files as you suggested.
thanks again.

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Apr 26, 2015

Contributor

❤️

Contributor

chancancode commented Apr 26, 2015

❤️

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Apr 27, 2015

Contributor

@SamSaffron good to merge this sooner rather than later otherwise it will get a bunch of conflicts with moving tests on master.

Contributor

arthurnn commented Apr 27, 2015

@SamSaffron good to merge this sooner rather than later otherwise it will get a bunch of conflicts with moving tests on master.

let :provider do
SiteSettings::LocalProcessProvider.new
end
def new_settings(provider)
- c = Class.new
- c.class_eval do
+ c = Class.new do

This comment has been minimized.

@SamSaffron

This comment has been minimized.

Show comment
Hide comment
@SamSaffron

SamSaffron Apr 27, 2015

Member

Thanks so much !!! ❤️

Member

SamSaffron commented Apr 27, 2015

Thanks so much !!! ❤️

SamSaffron added a commit that referenced this pull request Apr 27, 2015

@SamSaffron SamSaffron merged commit 6a338af into discourse:master Apr 27, 2015

1 check passed

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

@arthurnn arthurnn deleted the rspec_up branch Apr 28, 2015

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