The problem was that the change didn't break any tests because the tests overly stubbed node and weren't really testing anything. Specifically the line
- @node.expects(:parameters=).with "a" => "b", "c" => "d" in theory was testing that you should be able to assign parameters to a node, but it wasn't really testing that doing so worked, just that this assignment was called. The problem is that when it was called in a real puppet run it didn't work and gave the error that you find in this ticket, and the test doesn't catch that despite the test name "should set the resulting parameters as the node parameters". We changed the tests by using non-mock objects so that parameters assignment would fail, then we made the implementation change (changing parameters to an accessor instead of a reader) to get the tests to pass. We could have written additional tests, but they would have looked almost exactly the same as the tests we modified just without a mocked object. We made similar changes for the other attr_accessors on the class even though they weren't broken now, but if anyone changed them to readers in the future they wouldn't have the same problem that parameters did. We left fact merge stubbed on the test node object because that was a sufficiently complex method that we made sure was being tested elsewhere. I think this is a problem elsewhere in our tests that we overly stub simple objects for very little benefit. Sometimes there's just as many lines of code for stubbing and expecting things as there would be for just setting up the object and asserting what the object actually looked like at the end instead of what methods were called on it. Matt On Fri, Jul 16, 2010 at 11:38 PM, Markus Roberts <[email protected]>wrote: > Provisional +1 on the change itself, but I'm curious about why there > are all the test changes; if the rationale is sound it shouldn't have > broken any tests, right? It may have indicated the need for > additional tests, but it shouldn't have changed existing behavior. > Thus I"m left wondering if these are non-crucial changes (to be > strongly avoided at this stage of the release process) or the > rationale is suspect. > > Any idea which it is? > > On Fri, Jul 16, 2010 at 3:27 PM, Matt Robinson <[email protected]> > wrote: > > Node parameters were made a reader instead of an accessor in > > > > commit b82b4ef04282ca0006931562f60459a1591b6268 > > Author: Luke Kanies <[email protected]> > > Date: Wed Jan 6 17:42:42 2010 -0800 > > > > All non-transient parser references are gone > > > > but external nodes needs to be able to assign to parameters. The fix is > > just to change that back to an accessor. There may have been concern > > over nodes replacing the hash object instead of the values could have > > bad consequences, but that's not a concern since the node object being > > created in this case is new also. > > > > Paired with: Nick Lewis > > > > Signed-off-by: Matt Robinson <[email protected]> > > --- > > lib/puppet/node.rb | 4 ++-- > > spec/unit/indirector/node/exec_spec.rb | 9 +++++---- > > 2 files changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/lib/puppet/node.rb b/lib/puppet/node.rb > > index 1fc6154..2453cd1 100644 > > --- a/lib/puppet/node.rb > > +++ b/lib/puppet/node.rb > > @@ -16,8 +16,8 @@ class Puppet::Node > > indirects :node, :terminus_setting => :node_terminus, :doc => "Where to > find node information. > > A node is composed of its name, its facts, and its environment." > > > > - attr_accessor :name, :classes, :source, :ipaddress > > - attr_reader :time, :parameters > > + attr_accessor :name, :classes, :source, :ipaddress, :parameters > > + attr_reader :time > > > > def environment > > return super if @environment > > diff --git a/spec/unit/indirector/node/exec_spec.rb > b/spec/unit/indirector/node/exec_spec.rb > > index d5299ad..d214a5e 100755 > > --- a/spec/unit/indirector/node/exec_spec.rb > > +++ b/spec/unit/indirector/node/exec_spec.rb > > @@ -25,8 +25,9 @@ describe Puppet::Node::Exec do > > > > describe "when handling the results of the command" do > > before do > > - @node = stub 'node', :fact_merge => nil > > @name = "yay" > > + @node = Puppet::Node.new(@name) > > + @node.stubs(:fact_merge) > > Puppet::Node.expects(:new).with(@name).returns(@node) > > @result = {} > > # Use a local variable so the reference is usable in the execute > definition. > > @@ -45,14 +46,14 @@ describe Puppet::Node::Exec do > > > > it "should set the resulting parameters as the node parameters" do > > @result[:parameters] = {"a" => "b", "c" => "d"} > > - @node.expects(:parameters=).with "a" => "b", "c" => "d" > > @searcher.find(@request) > > + @node.parameters.should == {"a" => "b", "c" => "d"} > > end > > > > it "should set the resulting classes as the node classes" do > > @result[:classes] = %w{one two} > > - @node.expects(:classes=).with %w{one two} > > @searcher.find(@request) > > + @node.classes.should == [ 'one', 'two' ] > > end > > > > it "should merge the node's facts with its parameters" do > > @@ -62,8 +63,8 @@ describe Puppet::Node::Exec do > > > > it "should set the node's environment if one is provided" do > > @result[:environment] = "yay" > > - @node.expects(:environment=).with "yay" > > @searcher.find(@request) > > + @node.environment.to_s.should == 'yay' > > end > > end > > end > > -- > > 1.7.1 > > > > -- > > You received this message because you are subscribed to the Google Groups > "Puppet Developers" group. > > To post to this group, send email to [email protected]. > > To unsubscribe from this group, send email to > [email protected]<puppet-dev%[email protected]> > . > > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > > > > > > > > -- > ----------------------------------------------------------- > The power of accurate observation is > commonly called cynicism by those > who have not got it. ~George Bernard Shaw > ------------------------------------------------------------ > > -- > You received this message because you are subscribed to the Google Groups > "Puppet Developers" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]<puppet-dev%[email protected]> > . > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > > -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
