New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to RSpec 3 and major specs refactoring #87

Merged
merged 11 commits into from Apr 12, 2015

Conversation

Projects
None yet
3 participants
@mdesantis
Contributor

mdesantis commented Sep 3, 2014

Hello,

I'm planning to work a bit on rack-protection performance, so I cloned the project and I found the specs a bit cluttered; so I upgraded to RSpec 3 and did a cleanup.

These are my edits:

  • 022f994: Convert RSpec syntax from RSpec 2 to RSpec 3 using the wonderful tool Transpec
  • 2674740: Upgrade development dependency from RSpec 2 to RSpec 3
  • de797ce: Move spec files from a flat spec/*_spec.rb pattern to the conventional pattern lib/folder/file.rb -> spec/lib/folder/file_spec.rb
  • 01902df: Remove some define_methods on Struct.new classes in favour of Struct.new do; def ... ; end syntax
  • 941fee8: Remove execution permissions to some spec files
  • ed57eb2: Fix Ruby warnings visible using rspec --warning
  • ecae614: Fix a test which implementation was previously broken (more on this later)
  • 8196555: Remove unnedeed require 'stringio' line
  • c5020a2: Reorganize spec directives which previously were collected in spec_helper.rb into relative spec/support/*.rb files
  • a00a021: Generate rspec init default files with rspec --init and merge the previous ones into them
  • 513abea: Remove require 'spec_helper' not needed anymore (because .rspec now includes --require spec_helper option)

The only change which involves a change to a test is ecae614: I replaced the previous incorrect use of should inside a define_method with a mock, and then realized that the test was broken, because the changer middleware was called after the detector; so I switched them and fixed the test.

I hope my changes to be appreciated :)

mdesantis added some commits Sep 2, 2014

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

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

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

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

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

* 2 conversions
    from: be_false
      to: be_falsey

* 1 conversion
    from: be_true
      to: be_truthy

* 1 conversion
    from: obj.should_not_receive(:message)
      to: expect(obj).not_to receive(:message)

* 1 conversion
    from: obj.should_receive(:message)
      to: expect(obj).to receive(:message)

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

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Sep 3, 2014

Member

zomg

Member

zzak commented Sep 3, 2014

zomg

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Sep 3, 2014

Member

All of the spec changes look good, but could you explain the spec_help and spec/support stuff?
It's not obvious by looking at the diff

Member

zzak commented Sep 3, 2014

All of the spec changes look good, but could you explain the spec_help and spec/support stuff?
It's not obvious by looking at the diff

@mdesantis

This comment has been minimized.

Show comment
Hide comment
@mdesantis

mdesantis Sep 4, 2014

Contributor

c5020a2 actually consists of two edits (I noticed it now, it would be better if I made two commits for them):

  1. Rename TestHelpers into SpecHelpers: I made this because TestHelpers would be a good name for MiniTest, etc., instead SpecHelpers better suites for RSpec I think. I'm noticing that RSpec itself uses RSpecHelpers, maybe we can change to RSpecHelpers.

  2. Move RSpec related support code to spec/support folder: usually inside spec_helper.rb you have

    1. RSpec configuration (the RSpec.configure { |config| ... } block)

    2. Various code used to assist specs (spec helpers, monkey patches, shared examples...):

      stdlib and gems requires...
      
      Rack monkey patch code...
      
      DummyApp code...
      
      TestHelpers code...
      
      NotImplementedAsPending code...
      
      Rspec.configure { |config| ... }
      
      shared_examples code...

    I moved the support source code into respective spec/support/{feature_name}.rb files:

    So that spec_helper.rb now is:

    stdlib and gems requires...
    
    require 'support/rack_monkey_patches'
    require 'support/dummy_app'
    require 'support/spec_helpers'
    require 'support/not_implemented_as_pending'
    require 'support/shared_examples'
    
    Rspec.configure { |config| ... }

    But instead of write the various require 'support/...' I wrote

    Dir[File.expand_path('../support/**/*.rb', __FILE__)].each { |f| require f }

    which is similar to what is used by RSpec and similar to how RSpec sets Rails projects.

    I chose the support folder name because it's a RSpec convention: RSpec uses it for itself and RSpec Rails sets Rails projects to use support folder.

Contributor

mdesantis commented Sep 4, 2014

c5020a2 actually consists of two edits (I noticed it now, it would be better if I made two commits for them):

  1. Rename TestHelpers into SpecHelpers: I made this because TestHelpers would be a good name for MiniTest, etc., instead SpecHelpers better suites for RSpec I think. I'm noticing that RSpec itself uses RSpecHelpers, maybe we can change to RSpecHelpers.

  2. Move RSpec related support code to spec/support folder: usually inside spec_helper.rb you have

    1. RSpec configuration (the RSpec.configure { |config| ... } block)

    2. Various code used to assist specs (spec helpers, monkey patches, shared examples...):

      stdlib and gems requires...
      
      Rack monkey patch code...
      
      DummyApp code...
      
      TestHelpers code...
      
      NotImplementedAsPending code...
      
      Rspec.configure { |config| ... }
      
      shared_examples code...

    I moved the support source code into respective spec/support/{feature_name}.rb files:

    So that spec_helper.rb now is:

    stdlib and gems requires...
    
    require 'support/rack_monkey_patches'
    require 'support/dummy_app'
    require 'support/spec_helpers'
    require 'support/not_implemented_as_pending'
    require 'support/shared_examples'
    
    Rspec.configure { |config| ... }

    But instead of write the various require 'support/...' I wrote

    Dir[File.expand_path('../support/**/*.rb', __FILE__)].each { |f| require f }

    which is similar to what is used by RSpec and similar to how RSpec sets Rails projects.

    I chose the support folder name because it's a RSpec convention: RSpec uses it for itself and RSpec Rails sets Rails projects to use support folder.

@mdesantis

This comment has been minimized.

Show comment
Hide comment
@mdesantis

mdesantis Sep 10, 2014

Contributor

@zzak news about this pull request?

Contributor

mdesantis commented Sep 10, 2014

@zzak news about this pull request?

@mdesantis

This comment has been minimized.

Show comment
Hide comment
@mdesantis

mdesantis Mar 26, 2015

Contributor

News about this pull request? It seems to be abandoned... @rkh can I please ask your attention?

Contributor

mdesantis commented Mar 26, 2015

News about this pull request? It seems to be abandoned... @rkh can I please ask your attention?

@kytrinyx

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Mar 27, 2015

Member

@mdesantis I'm going to spend some time looking at rack-protection this weekend, and will figure out if there's something that needs to be done to get this merged.

There are 26 failing tests against jruby-head, with undefined method 'call' for #<#<Class:0x7752d052>:0x3468e86e>, but jruby-stable is passing. We'll need to figure out if it's a regression in jruby and report it there, or if there's some odd edge case that we are running into here.

Member

kytrinyx commented Mar 27, 2015

@mdesantis I'm going to spend some time looking at rack-protection this weekend, and will figure out if there's something that needs to be done to get this merged.

There are 26 failing tests against jruby-head, with undefined method 'call' for #<#<Class:0x7752d052>:0x3468e86e>, but jruby-stable is passing. We'll need to figure out if it's a regression in jruby and report it there, or if there's some odd edge case that we are running into here.

@mdesantis

This comment has been minimized.

Show comment
Hide comment
@mdesantis

mdesantis Mar 27, 2015

Contributor

Just curious... why do you test against heads? Are they supposed to be stable?

Contributor

mdesantis commented Mar 27, 2015

Just curious... why do you test against heads? Are they supposed to be stable?

@kytrinyx

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Mar 27, 2015

Member

They're not supposed to be stable, but it's a good way of catching new issues early (and help catch regressions in the language implementations)

Member

kytrinyx commented Mar 27, 2015

They're not supposed to be stable, but it's a good way of catching new issues early (and help catch regressions in the language implementations)

@kytrinyx

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Mar 30, 2015

Member

I got food poisoning, which screwed up my weekend plans pretty significantly. I haven't been able to get to any of what I had planned, unfortunately, but this is still at the top of my list.

Sorry about that.

Member

kytrinyx commented Mar 30, 2015

I got food poisoning, which screwed up my weekend plans pretty significantly. I haven't been able to get to any of what I had planned, unfortunately, but this is still at the top of my list.

Sorry about that.

@mdesantis

This comment has been minimized.

Show comment
Hide comment
@mdesantis

mdesantis Mar 30, 2015

Contributor

I got food poisoning on last Friday too :-S no problem, thank you for the
update

Maurizio De Santis

2015-03-30 4:55 GMT+02:00 Katrina Owen notifications@github.com:

I got food poisoning, which screwed up my weekend plans pretty
significantly. I haven't been able to get to any of what I had planned,
unfortunately, but this is still at the top of my list.

Sorry about that.


Reply to this email directly or view it on GitHub
#87 (comment)
.

Contributor

mdesantis commented Mar 30, 2015

I got food poisoning on last Friday too :-S no problem, thank you for the
update

Maurizio De Santis

2015-03-30 4:55 GMT+02:00 Katrina Owen notifications@github.com:

I got food poisoning, which screwed up my weekend plans pretty
significantly. I haven't been able to get to any of what I had planned,
unfortunately, but this is still at the top of my list.

Sorry about that.


Reply to this email directly or view it on GitHub
#87 (comment)
.

kytrinyx added a commit that referenced this pull request Apr 12, 2015

Merge pull request #87 from mdesantis/rspec3
Upgrade to RSpec 3 and major specs refactoring

@kytrinyx kytrinyx merged commit 90f6a49 into sinatra:master Apr 12, 2015

1 of 2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@kytrinyx

kytrinyx Apr 12, 2015

Member

I reran this against jruby-head, and it passed just fine. Not sure what happened back in the day. Thank you so much, @mdesantis, for taking the time to upgrade the specs, and I apologize for the delay in getting this merged.

Member

kytrinyx commented Apr 12, 2015

I reran this against jruby-head, and it passed just fine. Not sure what happened back in the day. Thank you so much, @mdesantis, for taking the time to upgrade the specs, and I apologize for the delay in getting this merged.

jeltz added a commit to jeltz/rack-protection that referenced this pull request Dec 15, 2015

zzak pushed a commit that referenced this pull request Aug 12, 2016

Merge pull request #87 from mdesantis/rspec3
Upgrade to RSpec 3 and major specs refactoring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment