any_instance stubs for library don't stick on Chef run #253

Closed
JeanMertz opened this Issue Nov 3, 2013 · 27 comments

Projects

None yet

4 participants

@JeanMertz

Using Chefspec 3.0.0, I seem to be unable to get any_instance stubs to persist during Chef runs. If I add a debug statement in the before block, I get the correct return value for every new instance I create:

$ Helpers::EncryptedSecrets.new({'secrets' => { 'encryption_version' => '20131102' }})
$ _.decrypt('test')
=> "password123"

As soon as the Chef-run starts, it somehow still enters the original method and the tests break.

require 'spec_helper'

describe 'base::_users' do
  let(:chef_instance) { ChefSpec::Runner.new }
  let(:chef_run)      { chef_instance.converge(described_recipe) }

  before do
    allow_any_instance_of(Helpers::EncryptedSecrets).to receive(:decrypt) {
      'password123'
    }
  end

  describe 'user_account[deployer]' do

    let(:chef_instance) do
      ChefSpec::Runner.new do |node|
        node.set['base']['user']['username'] = 'deployer'
      end
    end

    it 'creates a new user' do
      expect(chef_run).to create_user_account('deployer').with(
        password: 'password123',
      )
    end
  end
end

stacktrace:

  1) base::_users user_account[deployer] creates a new user
     Failure/Error: let(:chef_run)      { chef_instance.converge(described_recipe) }
     Chef::Exceptions::InvalidDataBagPath:
       Data bag path '/var/chef/data_bags' is invalid
     # /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-36046-a1uftw/base/libraries/encrypted_secrets.rb:37:in `encrypted_bag'
     # /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-36046-a1uftw/base/libraries/encrypted_secrets.rb:29:in `secret_key'
     # /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-36046-a1uftw/base/libraries/encrypted_secrets.rb:25:in `public_key'
     # /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-36046-a1uftw/base/libraries/encrypted_secrets.rb:19:in `decrypt'
     # /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-36046-a1uftw/base/recipes/_users.rb:24:in `block in from_file'
     # /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-36046-a1uftw/base/recipes/_users.rb:17:in `from_file'
     # ./spec/users_spec.rb:6:in `block (2 levels) in <top (required)>'
     # ./spec/users_spec.rb:33:in `block (3 levels) in <top (required)>'

Before, I stubbed everything required for the spec to pass, but it made the test 100 times slower (due to 2048bit encryption), so now I just want a fixed value returned.

@JeanMertz

Oh, here is my spec_helper, I am loading the required libraries as per #138:

require 'chefspec'
require 'chefspec/berkshelf'

requires = '{../libraries,support,shared}/**/*.rb'
Dir[File.join(File.dirname(__FILE__), requires)].each do |f|
  require File.expand_path(f)
end

RSpec.configure do |config|
  config.platform = 'debian'
  config.version  = '7.1'
end
@JeanMertz

And here is the actual recipe, in case it helps:

secret = Helpers::EncryptedSecrets.new(node)
user_details = node['base']['user']

chef_gem 'ruby-shadow'
user_account user_details['username'] do
  action    :create
  gid       user_details['group']
  uid       user_details['uid']
  home      user_details['home']
  shell     user_details['shell']
  password  secret.decrypt(user_details['password'])
end
@JeanMertz

And for completeness sake, here's the helper itself :)

require 'openssl'
require 'base64'

module Kabisa # :no-doc:
  class EncryptedSecrets
    attr_reader :version

    def initialize(node)
      @version = node['secrets']['encryption_version']
    end

    def encrypt(data)
      Base64.encode64 public_key.public_encrypt(data)
    end

    def decrypt(data)
      public_key.private_decrypt Base64.decode64(data)
    end

    private

    def public_key
      OpenSSL::PKey::RSA.new(secret_key, secret_password)
    end

    def secret_key
      encrypted_bag('secrets', version)['key']
    end

    def secret_password
      encrypted_bag('secrets', 'encryption_versions')[version]
    end

    def encrypted_bag(bag, item)
      Chef::EncryptedDataBagItem.load(bag, item)
    end
  end
end
@sethvargo
Owner

@JeanMertz can you confirm this works with ChefSpec 2? I suspect this is actually a Chef problem (because Chef is reloading the class after you've stubbed). Can you also post the output of the rspec run with the documentation formatter please?

@JeanMertz

@sethvargo here's the full stack, I will try version 2 later today:

$ rspec spec/users_spec.rb:26 --format documentation
Run options: include {:locations=>{"./spec/users_spec.rb"=>[26]}}

base::_users
  user_account[deployer]
WARN: Unresolved specs during Gem::Specification.reset:
      mini_portile (~> 0.5.0)
      pry (>= 0)
WARN: Clearing out unresolved specs.
Please report a bug if this causes problems.

================================================================================
Recipe Compile Error in /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-38609-ftwz0v/base/recipes/_users.rb
================================================================================

Chef::Exceptions::InvalidDataBagPath
------------------------------------
Data bag path '/var/chef/data_bags' is invalid

Cookbook Trace:
---------------
  /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-38609-ftwz0v/base/libraries/encrypted_secrets.rb:35:in `encrypted_bag'
  /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-38609-ftwz0v/base/libraries/encrypted_secrets.rb:27:in `secret_key'
  /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-38609-ftwz0v/base/libraries/encrypted_secrets.rb:23:in `public_key'
  /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-38609-ftwz0v/base/libraries/encrypted_secrets.rb:17:in `decrypt'
  /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-38609-ftwz0v/base/recipes/_users.rb:14:in `block in from_file'
  /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-38609-ftwz0v/base/recipes/_users.rb:7:in `from_file'

Relevant File Content:
----------------------
/var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-38609-ftwz0v/base/libraries/encrypted_secrets.rb:

 28:      end
 29:  
 30:      def secret_password
 31:        encrypted_bag('secrets', 'encryption_versions')[version]
 32:      end
 33:  
 34:      def encrypted_bag(bag, item)
 35>>       Chef::EncryptedDataBagItem.load(bag, item)
 36:      end
 37:    end
 38:  end
 39:  

    creates a new user (FAILED - 1)

Failures:

  1) base::_users user_account[deployer] creates a new user
     Failure/Error: let(:chef_run)      { chef_instance.converge(described_recipe) }
     Chef::Exceptions::InvalidDataBagPath:
       Data bag path '/var/chef/data_bags' is invalid
     # /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-38609-ftwz0v/base/libraries/encrypted_secrets.rb:35:in `encrypted_bag'
     # /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-38609-ftwz0v/base/libraries/encrypted_secrets.rb:27:in `secret_key'
     # /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-38609-ftwz0v/base/libraries/encrypted_secrets.rb:23:in `public_key'
     # /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-38609-ftwz0v/base/libraries/encrypted_secrets.rb:17:in `decrypt'
     # /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-38609-ftwz0v/base/recipes/_users.rb:14:in `block in from_file'
     # /var/folders/wn/5x5xky7d5h9bc28l4cw2jx3r0000gn/T/d20131103-38609-ftwz0v/base/recipes/_users.rb:7:in `from_file'
     # ./spec/users_spec.rb:5:in `block (2 levels) in <top (required)>'
     # ./spec/users_spec.rb:28:in `block (3 levels) in <top (required)>'

Top 1 slowest examples (1.16 seconds, 100.0% of total time):
  base::_users user_account[deployer] creates a new user
    1.16 seconds ./spec/users_spec.rb:27

Finished in 6.79 seconds
1 example, 1 failure

Failed examples:

rspec ./spec/users_spec.rb:27 # base::_users user_account[deployer] creates a new user
@ranjib
Collaborator
ranjib commented Nov 3, 2013

@JeanMertz can you try this:

 let(:chef_run) do
   ChefSpec::Runner.new("described_recipe") do |node|
     allow_any_instance_of(Helpers::EncryptedSecrets).to receive(:decrypt) {  'password123'}
   end.converge
 end
@JeanMertz

@ranjib I tried this, same results:

describe 'base::_users' do
  let(:chef_instance) { ChefSpec::Runner.new }
  let(:chef_run)      { chef_instance.converge(described_recipe) }

  describe 'user_account[deployer]' do

    let(:chef_instance) do
      ChefSpec::Runner.new do |node|
        node.set['base']['user']['username'] = 'deployer'
        allow_any_instance_of(Helpers::EncryptedSecrets).to receive(:decrypt) { 'password123' }
      end
    end

    it 'creates a new user' do
      expect(chef_run).to create_user_account('deployer').with(
        password: 'password123',
      )
    end
  end
end
@ranjib
Collaborator
ranjib commented Nov 3, 2013

i'm not sure which version of chef_instance is used by chef_run.

describe 'user_account[deployer]' do
  let(:runner) do
      ChefSpec::Runner.new do |node|
        node.set['base']['user']['username'] = 'deployer'
        allow_any_instance_of(Helpers::EncryptedSecrets).to receive(:decrypt) { 'password123' }
      end.converge('recipe_name')
    end

    it 'creates a new user' do
      expect(runner).to create_user_account('deployer').with(
        password: 'password123',
      )
    end
  end
@JeanMertz

@ranjib It should work, and I just commented out the initial chef_instance, just to make sure. No change though.

@JeanMertz

@sethvargo I tried downgrading to 2.x (which is quite a bit of work, once you're used to the nice workflow in 3.x ๐Ÿ‘ ). Unfortunately, it resulted in the same error as before.

Again, using the debugger, I could see the stub being set correctly, but still Chef doesn't pick it up when it runs.

@sethvargo
Owner

@JeanMertz okay, so I think you're stuck ๐Ÿ˜ฆ. Here's what's happening:

  1. You stub ๐Ÿ‘
  2. You make a ChefSpec::Runner ๐Ÿ‘
  3. You converge that runner ๐Ÿ‘
  4. Chef (because the ChefSpec::Runner actually runs Chef) reloads all your classes ๐Ÿ‘Ž
  5. Your stubs are gone

So, look at this simple test:

# something.rb
class Something
  def my_method
    puts "foo"
  end
end

# something_spec.rb

before do
  Something.stub(:my_method)
end

it 'stubs foo' do
  expect($stdout).to_not receive(:puts) # pass
  Something.new.my_method
end

it 'stubs foo (with reload)' do
  load './something.rb' # this reloads the class, so any stubs are overwritten :(
  expect($stdout).to_not receive(:puts)
  Something.new.my_method
end

There's actually nothing we can do in this case, unless I'm misunderstanding the situation. You could try using a hard stub (AKA monkey patch):

class Chef
  class EncryptedDataBag
    class << self
      alias_method :old_load, :load
      def load(*args); end
    end
  end
end

But I don't think that will work either.

@JeanMertz

@sethvargo I see your point. Too bad we're unable to stub these things in Chefspec due to the workings of Chef.

I've opted to use an (undocumented) node["TEST_RUN"] attribute to call a no-op encryption method.

This of course mixes test data into the production class, and can be dangerous if somehow used in production, but as long as it's undocumented, and only used during test runs where there is no other option, I think the tradeoffs are worth it.

As an aside, is there a way to force a specific attribute to always be the case in a Chefspec run, without defining it in the production files (so, f.e. define it in the Rspec config block), and be able to override it on a per-case basis?

Reasoning is that I now have to use node.set['secrets']['TEST_RUN'] = true for every chef_run converge that happens, whereas I'd rather set it to false on the tests that explicitly test this functionality.

# <snip>

    def initialize(node)
      @version  = node['secrets']['encryption_version']
      @test_run = node['secrets']['TEST_RUN']
    end

    def encrypt(data)
      test_run ? data : Base64.encode64(public_key.public_encrypt data)
    end

    def decrypt(data)
      test_run ? data : public_key.private_decrypt(Base64.decode64 data)
    end

# <snip>
@sethvargo
Owner

@JeanMertz

As an aside, is there a way to force a specific attribute to always be the case in a Chefspec run, without defining it in the production files (so, define it in the Rspec config block), and be able to override it on a per-case basis?

No, sorry. The node object isn't created until you converge, so that would not be possible.

@sethvargo sethvargo closed this Nov 4, 2013
@JeanMertz

@sethvargo Thanks for the help.

Regarding setting node attribute defaults. Since we're in control of Chefspec::Runner, wouldn't this be a nice feature to have? It could look for a specific rspec config to grab defaults, compare to the ones past into the runner's block, and merge the two (giving priority to the block defined attributes).

@sethvargo
Owner

@JeanMertz I don't think that's a good testing pattern we should follow. If you are setting node attributes, they should be on a per-test basis.

@JeanMertz

For anyone interested in this, here's a quick snippet to set some default attributes during a ChefSpec run, you can add this to your spec_helper:

module ChefSpec
  class Runner

    alias_method :orig_node, :node
    def node
      orig_node.tap { |node| node.override[:TEST_RUN] = true }
    end
  end
end
@rteabeault
Contributor

To the original issue: The problem is that chef reloads the cookbook files using Kernel.load. Our idea to get this to work was to monkey patch the cookbook compiler to not load the library files. As long as your test requires the library file it should already be loaded.

# spec_helper.rb

class Chef
  class RunContext
    class CookbookCompiler

      def load_libraries_from_cookbook(cookbook_name)
        files_in_cookbook_by_segment(cookbook_name, :libraries).each do |filename|
          begin
            Chef::Log.debug("Loading cookbook #{cookbook_name}'s library file: #{filename}")
            # Don't load library files.
            Kernel.load(filename) unless cookbook_name == 'my_cookbook'
            @events.library_file_loaded(filename)
          rescue Exception => e
            @events.library_file_load_failed(filename, e)
            raise
          end
        end
      end
    end
  end
end

We have only just tried this but it worked for our initial test. Also note that we had to tell it to only ignore library files in the cookbook we are testing. Perhaps chefspec itself could provide some means to bypass the loading of cookbook libraries if requested.

@sethvargo
Owner

@rteabeault that's a good workaround, but I don't feel comfortable adding something like that to Chef core

@rteabeault
Contributor

I assume you mean chefspec core. What I had in mind was maybe something you could add in your test like

allow_library_stubs_for('cookbook1', 'cookbook2')

While I understand not wanting to monkey patch chef code in the chefspec core, I feel like this is a shortcoming of chefspec. I want to be able to factor my helpers out into libraries and use them from within a provider. This allows one to test the helpers as pure ruby using plain rspec. But I still need to stub out those helpers when testing the provider itself. If I recall I heard you advocate this pattern during the discussion of testing LWRPs at the community summit. But without the ability to stub out the libraries I don't see how you can adequately do this.

@sethvargo
Owner

Just load the library manually before stubbing

@rteabeault
Contributor

I don't follow. As you pointed out before when you stub out things in libraries chef whacks all the stubs by calling Kernel.load. That is the purpose of the monkey patch I posted earlier. What am I missing?

@sethvargo
Owner

On each run, yes. But if you load the library, Chef won't load it again until the next Chef run.

@rteabeault
Contributor

Sorry if I am being dense here but how in a test do you get chefspec to have chef load the cookbook files without actually calling Chefspec::Runner.converge? I see how the following works due to the fact that we can actually delay the stubbing until after the converge is complete

# In some provider
action :my_action do
  ruby_block "Calling a library" do
    block do
      MyLibrary.do_something
    end
  end
end
# do some testing...
it "should call the library function" do
  chef_run = ChefSpec::Runner.new.converge descirbed_recipe
  MyLibrary.should_receive(:do_something)
  chef_run.find_resource(:ruby_block, "Calling a library").block.call
end

But what about this?

# my_provider.rb
action :my_action do
  some_value = MyHelper.compute_some_value
  # use some_value here
end

Now I want to stub out compute_some_value and assert that it gets used correctly later on. But when I do

it "should call the library function" do
  MyLibrary.stub(:compute_some_value).and_return(the_value)
  chef_run = ChefSpec::Runner.new(step_into: ['my_provider'].converge described_recipe
  # do some asserting here 
end

the stub gets overwritten when chef calls Kernel.load on the library files. And if I do

it "should call the library function" do
  chef_run = ChefSpec::Runner.new(step_into: ['my_provider'].converge described_recipe
  MyLibrary.stub(:compute_some_value).and_return(the_value)
  # do some asserting here 
end

then the action has already run and the stubbing is too late. So I think the answer is that you can't do this latter example using chefspec. At least not without doing some monkey patching as I had previously posted. So sorry if I am being dense but I don't see how you can stub in the latter example, converge and have the stub remain intact.

@sethvargo
Owner

@rteabeault I don't think you should test that way.

  1. Create a double object with the stubbed methods
  2. Stub the initializer for your class to return your stub
  3. Assert the stub receives the correct messages:
describe 'my_cookbook::default' do
  let(:chef_run) { ChefSpec::Runner.new.converge(described_recipe) }
  let(:my_library) { double('my_library', some_method: 'foo', other_method: 'bar') }

  # If you get an uninitialized constant warning for MyLibrary (which you might depending on a number of factors),
  # just `load mylibrary.rb` first
  before { MyLibrary.stub(:new).and_return(my_library) }

  it 'invokes #some_method' do
    expect(my_library).to receive(:some_method)
    chef_run
  end
end

That will handle both of your use cases.

@rteabeault
Contributor

Yes that works and I appreciate the help clearing this up. I know see that it works because new belongs to Class and does not get reloaded when chef calls Kernel.load on the library files. So what we are saying then is that if you want to write cookbooks with helper classes and test them with chefspec then you must do so using instances of a helper. Class methods and mixins are not going to be stubbable due to chef calling Kernel.load when compiling the cookbook. I must say thought that it would be nice to be able to stub class methods or mixins.

@rteabeault
Contributor

Ok. What about this? Add an optional block parameter to Chef::Runner#converge like so...

# runner.rb
def converge(*recipe_names)
  node.run_list.reset!
  recipe_names.each { |recipe_name| node.run_list.add(recipe_name) }

  return self if dry_run?

  # Expand the run_list
  expand_run_list!

  # Setup the run_context
  @run_context = client.setup_run_context

  # Allow stubbing/mocking after the cookbook has been compiled but before the converge
  yield if block_given?

  @converging = true
  @client.converge(@run_context)
  self
end

Then you could do

describe 'unit_test::class_helper' do
  let(:chef_run) { ChefSpec::Runner.new(:step_into => ['stub_test']) }

  it "should do something" do
    chef_run.converge(described_recipe) do
      ClassHelper.stub(:help_me).and_return('stubbed class helper')
    end

    # assert some stuff here
  end
end

No chef monkey patching and now you should be able to mock both Class helper methods as well as mixins.

@sethvargo
Owner

Sure, that works. PRs welcome

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