chef-boneyard/minitest-chef-handler

Spec test that checks file with incorrect (non-passing) mode fails with ArgumentError

leifmadsen opened this issue · 16 comments

leifmadsen commented

So I found this a little odd today. I created this spec test:

    it "ensures sshd_config file exists with correct permissions" do
      file("/etc/ssh/sshd_config").must_exist.with(:owner, "root").and(:group, "root").and(:mode, "644")
    end

When the mode of the file is not equal to 644 (mode 600 in this case) then I get the following:

  1. Error:
    test_0003_ensures_ssh_config_file_exists_with_correct_permissions(recipe::voiceaxis::sshd::files):
    ArgumentError: wrong number of arguments (2 for 1)
    /usr/local/lib/ruby/gems/1.9.1/gems/chef-10.14.4/lib/chef/resource/file.rb:85:in `diff'

If you look at the chef/resources/file.rb file, then you can see the 'diff' function only requires a single argument.

When the mode check is correct, then the output shows as a valid test, so you wouldn't notice it not working.

I've looked through the code some, and I can't seem to see what exactly is causing a 2nd argument to be passed to the 'diff' function.

leifmadsen commented

I realize I should probably show the backtrace, which doesn't appear to be all that useful. (I need to figure out how to enable better backtraces.)

"exception": "RuntimeError: MiniTest failed with 0 failure(s) and 1 error(s).",
"backtrace": [
  "/usr/local/lib/ruby/gems/1.9.1/gems/minitest-chef-handler-0.6.2/lib/minitest-chef-handler.rb:46:in `block in report'",
  "/usr/local/lib/ruby/gems/1.9.1/gems/chef-10.14.4/lib/chef/client.rb:110:in `call'",
  "/usr/local/lib/ruby/gems/1.9.1/gems/chef-10.14.4/lib/chef/client.rb:110:in `block in run_completed_successfully'",
  "/usr/local/lib/ruby/gems/1.9.1/gems/chef-10.14.4/lib/chef/client.rb:109:in `each'",
  "/usr/local/lib/ruby/gems/1.9.1/gems/chef-10.14.4/lib/chef/client.rb:109:in `run_completed_successfully'",
  "/usr/local/lib/ruby/gems/1.9.1/gems/chef-10.14.4/lib/chef/client.rb:426:in `do_run'",
  "/usr/local/lib/ruby/gems/1.9.1/gems/chef-10.14.4/lib/chef/client.rb:176:in `run'",
  "/usr/local/lib/ruby/gems/1.9.1/gems/chef-10.14.4/lib/chef/application/client.rb:283:in `block in run_application'",
  "/usr/local/lib/ruby/gems/1.9.1/gems/chef-10.14.4/lib/chef/application/client.rb:270:in `loop'",
  "/usr/local/lib/ruby/gems/1.9.1/gems/chef-10.14.4/lib/chef/application/client.rb:270:in `run_application'",
  "/usr/local/lib/ruby/gems/1.9.1/gems/chef-10.14.4/lib/chef/application.rb:70:in `run'",
  "/usr/local/lib/ruby/gems/1.9.1/gems/chef-10.14.4/bin/chef-client:26:in `<top (required)>'",
  "/usr/local/bin/chef-client:23:in `load'",
  "/usr/local/bin/chef-client:23:in `<main>'"
]
pabelanger commented

Does the following work?

describe "only root can modify the config file - alternate syntax" do
  let(:config) { file("/etc/ssh/sshd_config") }
  it { config.must_have(:mode, "644") }
  it { config.must_have(:owner, "root") }
  it { config.must_have(:group, "root") }
end
pabelanger commented

How about this?

it "ensures sshd_config file exists with correct permissions" do
  file("/etc/ssh/sshd_config").must_have(:mode, "644").with(:owner, "root").and(:group, "root")
end
leifmadsen commented

Tested your suggestions, and I can confirm the same output, which does help validate that it's a bug somewhere. The problem is the trace isn't useful in helping us really determine what is calling the function incorrectly. Once I have a way to do that, then I suspect the bug fix itself will be trivial.

leifmadsen commented

Curious if anyone has thoughts on how I might track this down? Got stuck trying to figure out what part in the chain path might be causing the issue. Thought appreciated!

leifmadsen commented

Well I got chef hacked up and know what is being passed now. There are two arguments being passed to the diff function in the file.rb file. It only accepts 1 argument but it is getting 2.

The values it is getting are the preferred and actual mode's of the file.

I added a Chef::Log.info() to the file.rb and get this:

[2012-11-16T14:05:51-05:00] INFO: Got arg: 600 and arg2: 644

I will continue to track down what is causing the multiple values, or whether the diff function should accept 2 arguments when it only gets 1 right now.

leifmadsen commented

There appears to be a bug somewhere in either the usage of 'assert_equal' or perhaps a bug in the implementation of 'assert_equal' in the MiniTest::Assert library.

When things match, I do not get the Chef::Log.info() message I added to the diff function in Chef proper. When they do not match, then I do get the message.

This seems to lead me to believe the diff function in Chef proper should never really be getting triggered for these tests. Something is happening in an implementation that is causing it to be used when it probably shouldn't be.

Here is the step through of the various lines and usage that is causing the error here. This is not related to just the 'mode' attribute, but pretty much anything to do with the 'with' resource. I get the same thing when using :owner etc....

Here is the trace through the related applications I've been testing with:

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

https://github.com/seattlerb/minitest/blob/master/lib/minitest/unit.rb#L230

http://bfts.rubyforge.org/minitest/MiniTest/Assertions.html#method-i-assert_equal

https://github.com/opscode/chef/blob/master/lib/chef/resource/file.rb#L85

leifmadsen commented

I hate to ping people directly, but perhaps @acrmp or @calavera have ideas on what could be happening here?

acrmp commented

Hi Leif,

Thanks a lot for persevering and debugging this. In the backtrace you can see Chef::Resource::File#diff is being called from MiniTest's assert_equal method.

This is because we mix the MiniTest::Assertions module into the Chef::Resource superclass. When #assert_equal looks for the #diff method expecting to find the method built-in to MiniTest it instead finds Chef's own support for diffing resources and blows up.

The fix should be to avoid including MiniTest::Assertions into Chef::Resource directly because we don't really need to do this.

Cheers,

Andrew.

leifmadsen commented

@acrmp thanks for the follow up! Now it all makes perfect sense. I'll give a shot at trying to enhance that section of code and determine what areas really need to be mixed into Chef::Resource (if anything at all).

leifmadsen commented

@acrmp @calavera sorry I'll have to leave it to you guys to resolve or someone else who comes along. I've hacked at this, but I don't understand enough about modules, mixins, methods, and how to break this out correctly.

kwilczynski commented

This is an important bug to fix. I have a lot of tests, as I test each and every cookbook / recipe, hence this is super annoying. I'd appreciate a fix :)

acrmp commented

Sorry, I should have chimed in earlier with some more detail for Leif. The suggested fix was to avoid including MiniTest::Assertions and to instead use composition for access to the assertions.

Within the #with method we would instead do something like:

mt = Object.extend(MiniTest::Assertions)
mt.assert_equal values, actual_values,
  "The #{resource_name} does not have the expected #{attribute}"

@leifmadsen - Can you give this a try? If you can get it working would you mind putting together a pull request?

I also wanted to write a spec to verify the methods exposed on Chef::Resource so we won't step on Chef's own methods in the future.

Thanks again for your dogged persistence!

Cheers,

Andrew.

leifmadsen commented

@acrmp that's awesome thanks! I will check that out and see what I can come up with. I should probably be able to figure that out based on your info and then I'll create a pull request.

A set of spec tests to check things would be a great idea. If you have some hints as to how you were going to approach that I can likely write the tests as well.

Thanks very much for continuing to follow up on this!

leifmadsen commented

#40 is the pull request that resolves this issue. Thanks!

calavera commented

Fixed.