Add Rspec tests for util/warnings.rb

Added in commit [8fe1028d41cec3935d1b8c5ddbc091253ad6f214] of branch fix-1233

All I see in that commit is this:

commit 8fe1028d41cec3935d1b8c5ddbc091253ad6f214
Author: Paul Lathrop <paul@tertiusfamily.net>
Date:   Mon May 19 21:11:10 2008 -0700

    Adding a clear_warnings method so I can do clean tests. Adding RSpec unit tests for Puppet::Util::Warnings

diff --git a/lib/puppet/util/warnings.rb b/lib/puppet/util/warnings.rb
index d009c39..2d1f143 100644
--- a/lib/puppet/util/warnings.rb
+++ b/lib/puppet/util/warnings.rb
@@ -10,5 +10,9 @@ module Puppet::Util::Warnings
             $stampwarnings[self.class] << msg
         end
     end
+
+    def clear_warnings()
+        $stampwarnings = {}
+    end
 end

Hah! I fail at Git.

commit [5591c94f2e3e6f8d8e3108956aec80f80761d26c] (branch fix-1233) adds in the actual tests.

These are a good start, but I would modify them slightly.

First, the return value of the method doesn't matter at all, so you should probably just return nil consistently, rather than returning the array.

Second, it's probably a better idea to test that the warnings don't repeat, rather than testing anything related to the involved array. Obviously, you'll still need to clear the array between tests, but then your interface becomes "make a warning" and "clear the list of warnings". It's best if the tests are completely unaware of the implementation, which should be pretty straightforward for this test.

Replying to luke:

First, the return value of the method doesn't matter at all, so you should probably just return nil consistently, rather than returning the array.

Okay. I was just testing what was there, rather than fixing it. I've been looking at this from a point of view where the tests get created, and point out flaws in the implementation; then the implementation flaws can be fixed, not necessarily by me.

It seems it makes more sense to fix things as I see them. I'll try to do that going forward.

Second, it's probably a better idea to test that the warnings don't repeat, rather than testing anything related to the involved array. Obviously, you'll still need to clear the array between tests, but then your interface becomes "make a warning" and "clear the list of warnings". It's best if the tests are completely unaware of the implementation, which should be pretty straightforward for this test.

Without the return value, how would I test the warnings don't repeat? My first pass at it would probably involve mocking Puppet.warning...

I'll take another stab at this again later this week.

To test that the warnings don't repeat, you'd use something like this:

it "should only send a given warning once" do
  Puppet.expects(:warning).once
  object.warnonce "this is a test"
  object.warnonce "this is a test"
end

Replying to luke:

To test that the warnings don't repeat, you'd use something like this: {{{ it "should only send a given warning once" do Puppet.expects(:warning).once object.warnonce "this is a test" object.warnonce "this is a test" end }}}

That's kinda what I thought. I've refactored, but Git Hub? is down - I'll push when it comes back.

commit 5727ba6efc33a84014231d001a9168896aa0fea7 of branch fix-1233 on Git Hub?

reassigning to James because my patches are against 0.24.x

Pushed in commit [d55a75525ad931aaeab87fd9173f9e92df000ed9] in branch 0.24.x

Okay, for real this time.

commit [424b0d974823838ce56cdee72fe87ed057ef95a4] of branch fix-1233

Pushed in commit [c173264408e8f2dbb7a72e984a4cbce4637f3c0c] in branch 0.24.x