chef-boneyard / minitest-chef-handler

Run minitest suites after your Chef recipes to check the status of your system.
Other
163 stars 44 forks source link

TypeError: no implicit conversion from nil to integer #23

Closed leifmadsen closed 12 years ago

leifmadsen commented 12 years ago

When running with Chef 0.10.12, and minitest-handler version 0.0.10, with a very simple test via a private chef server, I get this on a CentOS 6.2 server:

Run options: -v --seed 49948

# Running tests:

recipe::system_test::default::system files ok#test_0001_passwd_file_has_exists_with_correct_perms = 
0.00 s = E

Finished tests in 0.003376s, 296.2085 tests/s, 296.2085 assertions/s.

  1) Error:
test_0001_passwd_file_has_exists_with_correct_perms(recipe::system_test::default::system files ok):
TypeError: no implicit conversion from nil to integer
    /var/chef/minitest/autotesting/system_test.rb:14:in `test_0001_passwd_file_has_exists_with_correct_perms'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/handler.rb:218:in `run_report_unsafe'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/handler.rb:206:in `run_report_safely'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/handler.rb:91:in `run_report_handlers'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/handler.rb:90:in `each'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/handler.rb:90:in `run_report_handlers'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/handler.rb:99
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/client.rb:104:in `call'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/client.rb:104:in `run_completed_successfully'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/client.rb:103:in `each'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/client.rb:103:in `run_completed_successfully'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/client.rb:168:in `run'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/application/client.rb:254:in `run_application'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/application/client.rb:241:in `loop'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/application/client.rb:241:in `run_application'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/../lib/chef/application.rb:70:in `run'
    /usr/lib64/ruby/gems/1.8/gems/chef-10.12.0/bin/chef-client:26
    /usr/bin/chef-client:23:in `load'
    /usr/bin/chef-client:23

1 tests, 1 assertions, 0 failures, 1 errors, 0 skips

The run_list looks like:

  "run_list": [
    "recipe[minitest-handler]",
    "recipe[autotesting::default]"
  ]

And the autotesting cookbook was crated with 'knife cookbook create autotesting', followed by a 'mkdir -p site-cookbooks/autotesting/files/default/tests/minitest/'

I then added a new 'system_test.rb' file to the minitest directory with the following contents:

require "rubygems"
require 'minitest/spec'
require 'minitest/autorun'

describe_recipe 'system_test::default' do

    include MiniTest::Chef::Assertions
    include MiniTest::Chef::Context
    include MiniTest::Chef::Resources

    describe "system files ok" do
        it "passwd file has exists with correct perms" do
            file("/etc/passwd").must_exist.with(:owner, "root")
        end
    end
end

However the test will pass if I strip off the .with(:owner, "root") from the file resource.

leifmadsen commented 12 years ago

Also, I copy/pasta'd the spec_examples::default just to make sure I didn't have some weird syntax issue, but it appears there as well. Here is the copy/pasta I'm testing with as well:

require 'minitest/spec'

describe_recipe 'spec_examples::default' do

  # It's often convenient to load these includes in a separate helper along with
  # your own helper methods, but here we just include them directly:
  include MiniTest::Chef::Assertions
  include MiniTest::Chef::Context
  include MiniTest::Chef::Resources

  describe "files" do

    # = Testing that a file exists =
    #
    # The simplest assertion is that a file exists following the Chef run:
    it "creates the config file" do
      file("/etc/fstab").must_exist
    end

    # All of the matchers starting with 'must_' also have a negative 'wont_'.
    # So conversely we can also check that a file does not exist:
    it "ensures that the foobar file is removed if present" do
      file("/etc/foobar").wont_exist
    end

    it "has the expected ownership and permissions" do
      file("/etc/fstab").must_exist.with(:owner, "root")
    end
  end
end
leifmadsen commented 12 years ago

Additional testing with Ubuntu 12.04 as well; same error.

I believe the problem may lie somewhere around like 24 of lib/minitest-chef-handler/resources.rb as that is where 'with' is defined.

calavera commented 12 years ago

Hi @leifmadsen, thanks for reporting this problem.

At first glance, it seems your file resource doesn't have an owner, which can be a Chef bug, I haven't used the version 0.10.12 yet because I found the version 0.10.10 super buggie. You can confirm this printing the owner in the spec with something like this:

p file('/etc/fstab').owner

Either way, I'm not sure if our code to extract the name of the owner is the most correct either. As you can see in the method below, we assume Chef stores the owner as a UID (Integer), but if the implementation changes and they decide to store the name (String) the method will fail because Etc.getpwuid expects an integer.

https://github.com/calavera/minitest-chef-handler/blob/master/lib/minitest-chef-handler/resources.rb#L41-48

I'm going to modify that method to verify how the owner is stored first. If you could confirm that your resource has actually an owner I would really appreciate it.

leifmadsen commented 12 years ago

is that p file(...).owner done in the resource.rb file?

calavera commented 12 years ago

nah, I mean to put that in your spec. Something like:

it "has the expected ownership and permissions" do
  p file('/etc/fstab').owner
  file("/etc/fstab").must_exist.with(:owner, "root")
end

So we are sure the resource has an owner.

calavera commented 12 years ago

Internally the with method calls owner to verify that your expectation is right. I think the problem is that we're passing nil to Etc.getpwuid and that's why the spec is failing.

leifmadsen commented 12 years ago

I think you're right:

recipe::spec_examples::default::files#test_0003_has_the_expected_ownership_and_permissions = "-------------------------------"
nil
"0000000000000000000000000000000"
      p("-------------------------------")
      p file('/etc/fstab').owner
      p("0000000000000000000000000000000")
leifmadsen commented 12 years ago

Additionally, I noticed the .mode method fails as well, as it returns "" instead of "644" like I would expect. However, the test doesn't seem to actually fail:

recipe::spec_examples::default::files#test_0004_should_check_the_permissions_of_the_file = "File mode: "
nil
0.00 s = .

recipe::spec_examples::default::files#test_0003_has_the_expected_ownership_and_permissions = "File owner: "
nil
0.00 s = E
calavera commented 12 years ago

Yeah, we can do better and detect those cases in the with method, so it informs you when owner and mode are nil.

On Sunday, June 24, 2012 at 2:47 PM, Leif Madsen wrote:

Additionally, I noticed the .mode method fails as well, as it returns "" instead of "644" like I would expect. However, the test doesn't seem to actually fail: recipe::spec_examples::default::files#test_0004_should_check_the_permissions_of_the_file = "File mode: " nil 0.00 s = . recipe::spec_examples::default::files#test_0003_has_the_expected_ownership_and_permissions = "File owner: " nil 0.00 s = E

— Reply to this email directly or view it on GitHub (https://github.com/calavera/minitest-chef-handler/issues/23#issuecomment-6536660).

leifmadsen commented 12 years ago

I wish the .owner and .mode methods actually returned data :) Sounds like I need to go up a level to get this filed and potentially fixed I guess. The fact it is returning 'nil' is especially strange (on ruby 1.8.7 with CentOS 6.2 and 1.9.1 with Ubuntu 12.04).

I might go back to an earlier version of chef-client and just see what it returns if anything. Any recommendations for a version I should test out?

calavera commented 12 years ago

I use Chef 0.10.8 because the Link resource in Chef 0.10.10 was completely broken. I think it's fixed in 0.10.12 but I haven't had time to test this version.

On Sunday, June 24, 2012 at 2:59 PM, Leif Madsen wrote:

I wish the .owner and .mode methods actually returned data :) Sounds like I need to go up a level to get this filed and potentially fixed I guess. The fact it is returning 'nil' is especially strange (on ruby 1.8.7 with CentOS 6.2 and 1.9.1 with Ubuntu 12.04). I might go back to an earlier version of chef-client and just see what it returns if anything. Any recommendations for a version I should test out?

— Reply to this email directly or view it on GitHub (https://github.com/calavera/minitest-chef-handler/issues/23#issuecomment-6536753).

leifmadsen commented 12 years ago

Wow interesting.

If I go back to Chef 0.10.8, things work (and pass).

If I use either 0.10.10 or 0.10.12, the tests fail as reported here (so I suspect the changes in 0.10.12 do not address the issues you would expect).

btw: thank you for the quick responses

leifmadsen commented 12 years ago

Just for reference, the values returned:

recipe::spec_examples::default::files#test_0003_has_the_expected_ownership_and_permissions = "File owner: "
0
0.00 s = .
recipe::spec_examples::default::files#test_0004_should_check_the_permissions_of_the_file = "File mode: "
420
0.00 s = .
leifmadsen commented 12 years ago

Do you happen to know of the CHEF issue on the tracker I should follow up on? I did a search and can't find anything related. Don't want to file a duplicate. Thanks!

acrmp commented 12 years ago

It looks like #load_current_resource on the file resource no longer populates the owner and mode with the state of an existing file.

This behaviour changed in 0.10.10 when changes were made to genericise permissions for windows support. opscode/chef@4eba6949adc5dbabe5b054d42ecebb3fdeffc987

leifmadsen commented 12 years ago

@acrmp Thanks for finding that commit! I'm going to open a CHEF ticket with the commit as a link. Thanks!

leifmadsen commented 12 years ago

Filed as issue http://tickets.opscode.com/browse/CHEF-3235

leifmadsen commented 12 years ago

I'm curious if there are new, preferred methods to be using, or a different way that minitest-chef-handler should be utilizing the lookups for a file method. Unfortunately it's hard for me to determine if the way minitest-chef-handler is accessing the methods is deprecated, or if the way the methods were changed in Chef are incorrect.

calavera commented 12 years ago

@leifmadsen the Chef api is not really consistent. owner, group and mode and accessors in the resources while any other attribute is a method, that's why we have that case statement.

I've released the version 0.5.2 that guards the tests from this failure and shows instead something like this:

The file does not have the expected owner.
Expected: "david"
Actual: nil

Thanks for reporting this!

leifmadsen commented 12 years ago

Cool thanks! Hopefully in the future this will work correctly on something greater than Chef 0.10.8 as it looks like I'll be stuck on that for now.

Thanks for the quick responses and fix!

JeanMertz commented 11 years ago

@calavera I'm using Chef 11.6 with minitest-chef-handler, and still see this issue. That is, the error is now caught correctly (showing the error you provided above), but why is it showing this error? When I ssh into the box, I can see the proper user rights, but minitest-chef-handler still tells me it gets nil back?

scarolan commented 11 years ago

I am also seeing this error on Chef 11.8.