Update to RSpec 3 #2324

Merged
merged 27 commits into from Nov 7, 2014

Conversation

Projects
None yet
5 participants
@mcquin
Contributor

mcquin commented Oct 29, 2014

Sorry for jumbo PR. This is sorely needed for some upcoming work.

Used transpec to update most of the files. The major exceptions are covering the changes to pending. Generally, I use pending if an :if was supplied, and skip if a filter was given or pending was not given a block.

\cc @opscode/client-engineers

@lamont-granquist

This comment has been minimized.

Show comment
Hide comment
@lamont-granquist

lamont-granquist Oct 29, 2014

Contributor

lol, you broke github, too.

i see a bunch of spec/unit/{resource,provider} files that were covered in #2300 that seem to have zero lines of diffs. do you need to rebase onto origin/master?

Contributor

lamont-granquist commented Oct 29, 2014

lol, you broke github, too.

i see a bunch of spec/unit/{resource,provider} files that were covered in #2300 that seem to have zero lines of diffs. do you need to rebase onto origin/master?

end
it "returns true if registry has specified value" do
values = @resource.registry_get_values("HKCU\\Software\\Root")
- values.include?({:name=>"RootType1",:type=>:string,:data=>"fibrous"}).should == true
+ expect(values.include?({:name=>"RootType1",:type=>:string,:data=>"fibrous"})).to eq(true)

This comment has been minimized.

@tyler-ball

tyler-ball Oct 30, 2014

Contributor

The .include? calls could be rewritten as expect(values).to include({:name=>"RootType1",:type=>:string,:data=>"fibrous"}) - thats a valid matcher, right?

@tyler-ball

tyler-ball Oct 30, 2014

Contributor

The .include? calls could be rewritten as expect(values).to include({:name=>"RootType1",:type=>:string,:data=>"fibrous"}) - thats a valid matcher, right?

This comment has been minimized.

@mcquin

mcquin Oct 30, 2014

Contributor

Yep. Anything that responds to 'include?' works with the 'include' matcher.
On Oct 29, 2014 5:35 PM, "Tyler Ball" notifications@github.com wrote:

In spec/functional/dsl/registry_helper_spec.rb:

 end
 it "returns true if registry has specified value" do
   values = @resource.registry_get_values("HKCU\\Software\\Root")
  •  values.include?({:name=>"RootType1",:type=>:string,:data=>"fibrous"}).should == true
    
  •  expect(values.include?({:name=>"RootType1",:type=>:string,:data=>"fibrous"})).to eq(true)
    

The .include? calls could be rewritten as expect(values).to
include({:name=>"RootType1",:type=>:string,:data=>"fibrous"}) - thats a
valid matcher, right?


Reply to this email directly or view it on GitHub
https://github.com/opscode/chef/pull/2324/files#r19582214.

@mcquin

mcquin Oct 30, 2014

Contributor

Yep. Anything that responds to 'include?' works with the 'include' matcher.
On Oct 29, 2014 5:35 PM, "Tyler Ball" notifications@github.com wrote:

In spec/functional/dsl/registry_helper_spec.rb:

 end
 it "returns true if registry has specified value" do
   values = @resource.registry_get_values("HKCU\\Software\\Root")
  •  values.include?({:name=>"RootType1",:type=>:string,:data=>"fibrous"}).should == true
    
  •  expect(values.include?({:name=>"RootType1",:type=>:string,:data=>"fibrous"})).to eq(true)
    

The .include? calls could be rewritten as expect(values).to
include({:name=>"RootType1",:type=>:string,:data=>"fibrous"}) - thats a
valid matcher, right?


Reply to this email directly or view it on GitHub
https://github.com/opscode/chef/pull/2324/files#r19582214.

@@ -44,8 +44,8 @@
it "updates the evil laugh, even in why-run mode" do
new_resource.run_action(new_resource.action)
- $evil_global_evil_laugh.should == :mwahahaha
- new_resource.should be_updated
+ expect($evil_global_evil_laugh).to eq(:mwahahaha)

This comment has been minimized.

@tyler-ball

tyler-ball Oct 30, 2014

Contributor

what a great expectation 👍

@tyler-ball

tyler-ball Oct 30, 2014

Contributor

what a great expectation 👍

@@ -49,7 +49,7 @@ def registry_safe?
describe "when there is nothing to indicate a reboot is pending" do
it "should return false" do
pending "Found existing registry keys" unless registry_safe?
- expect(recipe.reboot_pending?).to be_false
+ expect(recipe.reboot_pending?).to be_falsey

This comment has been minimized.

@tyler-ball

tyler-ball Oct 30, 2014

Contributor

If there was already an existing correctly written expectation, shouldn't we leave this alone? In this case it looks like the method is meant to return true or false, so the be_false expectation seems correct.

@tyler-ball

tyler-ball Oct 30, 2014

Contributor

If there was already an existing correctly written expectation, shouldn't we leave this alone? In this case it looks like the method is meant to return true or false, so the be_false expectation seems correct.

This comment has been minimized.

@mcquin

mcquin Oct 30, 2014

Contributor

RSpec 2's be_true and be_false marchers were changed in RSpec 3 to
be_truthy and be_falsey to more correctly reflect the matcher's actual
behavior.

All that aside, yes a more accurate test would be for the exact boolean 'be
true' and 'be false'.
On Oct 29, 2014 5:42 PM, "Tyler Ball" notifications@github.com wrote:

In spec/functional/dsl/reboot_pending_spec.rb:

@@ -49,7 +49,7 @@ def registry_safe?
describe "when there is nothing to indicate a reboot is pending" do
it "should return false" do
pending "Found existing registry keys" unless registry_safe?

  •    expect(recipe.reboot_pending?).to be_false
    
  •    expect(recipe.reboot_pending?).to be_falsey
    

If there was already an existing correctly written expectation, shouldn't
we leave this alone? In this case it looks like the method is meant to
return true or false, so the be_false expectation seems correct.


Reply to this email directly or view it on GitHub
https://github.com/opscode/chef/pull/2324/files#r19582371.

@mcquin

mcquin Oct 30, 2014

Contributor

RSpec 2's be_true and be_false marchers were changed in RSpec 3 to
be_truthy and be_falsey to more correctly reflect the matcher's actual
behavior.

All that aside, yes a more accurate test would be for the exact boolean 'be
true' and 'be false'.
On Oct 29, 2014 5:42 PM, "Tyler Ball" notifications@github.com wrote:

In spec/functional/dsl/reboot_pending_spec.rb:

@@ -49,7 +49,7 @@ def registry_safe?
describe "when there is nothing to indicate a reboot is pending" do
it "should return false" do
pending "Found existing registry keys" unless registry_safe?

  •    expect(recipe.reboot_pending?).to be_false
    
  •    expect(recipe.reboot_pending?).to be_falsey
    

If there was already an existing correctly written expectation, shouldn't
we leave this alone? In this case it looks like the method is meant to
return true or false, so the be_false expectation seems correct.


Reply to this email directly or view it on GitHub
https://github.com/opscode/chef/pull/2324/files#r19582371.

This comment has been minimized.

@mcquin

mcquin Oct 30, 2014

Contributor

In short, my answer is "the spec wasn't written correctly in the first
case" and it will be updated.
On Oct 29, 2014 10:44 PM, "Claire McQuin" claire@getchef.com wrote:

RSpec 2's be_true and be_false marchers were changed in RSpec 3 to
be_truthy and be_falsey to more correctly reflect the matcher's actual
behavior.

All that aside, yes a more accurate test would be for the exact boolean
'be true' and 'be false'.
On Oct 29, 2014 5:42 PM, "Tyler Ball" notifications@github.com wrote:

In spec/functional/dsl/reboot_pending_spec.rb:

@@ -49,7 +49,7 @@ def registry_safe?
describe "when there is nothing to indicate a reboot is pending" do
it "should return false" do
pending "Found existing registry keys" unless registry_safe?

  •    expect(recipe.reboot_pending?).to be_false
    
  •    expect(recipe.reboot_pending?).to be_falsey
    

If there was already an existing correctly written expectation, shouldn't
we leave this alone? In this case it looks like the method is meant to
return true or false, so the be_false expectation seems correct.


Reply to this email directly or view it on GitHub
https://github.com/opscode/chef/pull/2324/files#r19582371.

@mcquin

mcquin Oct 30, 2014

Contributor

In short, my answer is "the spec wasn't written correctly in the first
case" and it will be updated.
On Oct 29, 2014 10:44 PM, "Claire McQuin" claire@getchef.com wrote:

RSpec 2's be_true and be_false marchers were changed in RSpec 3 to
be_truthy and be_falsey to more correctly reflect the matcher's actual
behavior.

All that aside, yes a more accurate test would be for the exact boolean
'be true' and 'be false'.
On Oct 29, 2014 5:42 PM, "Tyler Ball" notifications@github.com wrote:

In spec/functional/dsl/reboot_pending_spec.rb:

@@ -49,7 +49,7 @@ def registry_safe?
describe "when there is nothing to indicate a reboot is pending" do
it "should return false" do
pending "Found existing registry keys" unless registry_safe?

  •    expect(recipe.reboot_pending?).to be_false
    
  •    expect(recipe.reboot_pending?).to be_falsey
    

If there was already an existing correctly written expectation, shouldn't
we leave this alone? In this case it looks like the method is meant to
return true or false, so the be_false expectation seems correct.


Reply to this email directly or view it on GitHub
https://github.com/opscode/chef/pull/2324/files#r19582371.

This comment has been minimized.

@mcquin

mcquin Oct 30, 2014

Contributor

Additionally, I don't think RSpec 3 has 'be_false'.
On Oct 29, 2014 10:46 PM, "Claire McQuin" claire@getchef.com wrote:

In short, my answer is "the spec wasn't written correctly in the first
case" and it will be updated.
On Oct 29, 2014 10:44 PM, "Claire McQuin" claire@getchef.com wrote:

RSpec 2's be_true and be_false marchers were changed in RSpec 3 to
be_truthy and be_falsey to more correctly reflect the matcher's actual
behavior.

All that aside, yes a more accurate test would be for the exact boolean
'be true' and 'be false'.
On Oct 29, 2014 5:42 PM, "Tyler Ball" notifications@github.com wrote:

In spec/functional/dsl/reboot_pending_spec.rb:

@@ -49,7 +49,7 @@ def registry_safe?
describe "when there is nothing to indicate a reboot is pending" do
it "should return false" do
pending "Found existing registry keys" unless registry_safe?

  •    expect(recipe.reboot_pending?).to be_false
    
  •    expect(recipe.reboot_pending?).to be_falsey
    

If there was already an existing correctly written expectation,
shouldn't we leave this alone? In this case it looks like the method is
meant to return true or false, so the be_false expectation seems
correct.


Reply to this email directly or view it on GitHub
https://github.com/opscode/chef/pull/2324/files#r19582371.

@mcquin

mcquin Oct 30, 2014

Contributor

Additionally, I don't think RSpec 3 has 'be_false'.
On Oct 29, 2014 10:46 PM, "Claire McQuin" claire@getchef.com wrote:

In short, my answer is "the spec wasn't written correctly in the first
case" and it will be updated.
On Oct 29, 2014 10:44 PM, "Claire McQuin" claire@getchef.com wrote:

RSpec 2's be_true and be_false marchers were changed in RSpec 3 to
be_truthy and be_falsey to more correctly reflect the matcher's actual
behavior.

All that aside, yes a more accurate test would be for the exact boolean
'be true' and 'be false'.
On Oct 29, 2014 5:42 PM, "Tyler Ball" notifications@github.com wrote:

In spec/functional/dsl/reboot_pending_spec.rb:

@@ -49,7 +49,7 @@ def registry_safe?
describe "when there is nothing to indicate a reboot is pending" do
it "should return false" do
pending "Found existing registry keys" unless registry_safe?

  •    expect(recipe.reboot_pending?).to be_false
    
  •    expect(recipe.reboot_pending?).to be_falsey
    

If there was already an existing correctly written expectation,
shouldn't we leave this alone? In this case it looks like the method is
meant to return true or false, so the be_false expectation seems
correct.


Reply to this email directly or view it on GitHub
https://github.com/opscode/chef/pull/2324/files#r19582371.

This comment has been minimized.

@lamont-granquist

lamont-granquist Oct 30, 2014

Contributor

yeah that dropped that to force you to use "be false" or "be_falsey" -- "be_falsey" is the back-compat change.

lots of our "be_false" tests should probably change to "be false" to be more correct, but there's probably no good automated way to fix all of those.

@lamont-granquist

lamont-granquist Oct 30, 2014

Contributor

yeah that dropped that to force you to use "be false" or "be_falsey" -- "be_falsey" is the back-compat change.

lots of our "be_false" tests should probably change to "be false" to be more correct, but there's probably no good automated way to fix all of those.

@@ -1,2 +1,2 @@
--color
--fs

This comment has been minimized.

@tyler-ball

tyler-ball Oct 30, 2014

Contributor

Was s even a real formatter before? I don't see it listed as a valid formatter in my rspec --help output.

@tyler-ball

tyler-ball Oct 30, 2014

Contributor

Was s even a real formatter before? I don't see it listed as a valid formatter in my rspec --help output.

This comment has been minimized.

@lamont-granquist

lamont-granquist Oct 30, 2014

Contributor

Yep used to work in rspec 2.x

@lamont-granquist

lamont-granquist Oct 30, 2014

Contributor

Yep used to work in rspec 2.x

@tyler-ball

This comment has been minimized.

Show comment
Hide comment
@tyler-ball

tyler-ball Oct 30, 2014

Contributor

💯 * 💯

A grand undertaking - its looking great so far!

Contributor

tyler-ball commented Oct 30, 2014

💯 * 💯

A grand undertaking - its looking great so far!

Claire McQuin added some commits Oct 30, 2014

Claire McQuin
Merge branch 'master' into mcquin/rspec-3
Conflicts:
	spec/functional/dsl/reboot_pending_spec.rb
	spec/functional/event_loggers/windows_eventlog_spec.rb
@mcquin

This comment has been minimized.

Show comment
Hide comment
@mcquin

mcquin Oct 30, 2014

Contributor

RSpec is getting really upset that the helper methods called in the before and after blocks reference things set by :let.

RSpec is getting really upset that the helper methods called in the before and after blocks reference things set by :let.

This comment has been minimized.

Show comment
Hide comment
@lamont-granquist

lamont-granquist Oct 31, 2014

Contributor

I think let! was invented to fix that problem...

Contributor

lamont-granquist replied Oct 31, 2014

I think let! was invented to fix that problem...

@mcquin

This comment has been minimized.

Show comment
Hide comment
@mcquin

mcquin Nov 3, 2014

Contributor

@opscode/client-engineers Waiting on CVTs, this last change should fix the last of the failures (verified locally). Builds look happy. With #2346 the update to RSpec 3 should be complete (thanks, @tyler-ball)!

Contributor

mcquin commented Nov 3, 2014

@opscode/client-engineers Waiting on CVTs, this last change should fix the last of the failures (verified locally). Builds look happy. With #2346 the update to RSpec 3 should be complete (thanks, @tyler-ball)!

@mcquin

This comment has been minimized.

Show comment
Hide comment
@mcquin

mcquin Nov 7, 2014

Contributor

@opscode/client-engineers A GREEN CHECKMARK!

Contributor

mcquin commented Nov 7, 2014

@opscode/client-engineers A GREEN CHECKMARK!

@lamont-granquist

This comment has been minimized.

Show comment
Hide comment
@lamont-granquist

lamont-granquist Nov 7, 2014

Contributor

quick stab the merge button before it goes away!

Contributor

lamont-granquist commented Nov 7, 2014

quick stab the merge button before it goes away!

mcquin added a commit that referenced this pull request Nov 7, 2014

@mcquin mcquin merged commit 1888a8a into master Nov 7, 2014

1 check passed

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

@mcquin mcquin deleted the mcquin/rspec-3 branch Nov 21, 2014

@chef chef locked and limited conversation to collaborators Nov 16, 2017

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