On Tue, 22 Mar 2011 23:35:10 +0100, Stefan Schulte wrote: > [...]
> + The second way is the prefered way if you want to specifiy a port that
> + uses both tcp and udp as a protocol. You need to define two resources
> + for such a port but the resource title still has to be uniq.
Should probably be 'unique', not 'uniq', but this is easily fixed when
applying the patch, if it's not already done in the next patch series.
(Your cover letter made it sound like you'd have another round coming
with a test related to #5605.)
[...]
> + def self.title_patterns
> + [
> + # we have two title_patterns "name" and "name:protocol". We won't use
> + # one pattern (that will eventually set :protocol to nil) because we
> + # want to use a default value for :protocol. And that does only work
> + # if :protocol is not put in the parameter hash while initialising
> + [
> + /^(.*?)\/(tcp|udp)$/, # Set name and protocol
> + [
> + # We don't need a lot of post-parsing
> + [ :name, lambda{|x| x} ],
> + [ :protocol, lambda{ |x| x.intern unless x.nil? } ]
> + ]
> + ],
> + [
> + /^(.*)$/,
> + [
> + [ :name, lambda{|x| x} ]
> + ]
> + ]
> + ]
> + end
The original version also allowed 'ddp' (even though it wasn't
documented). Should this version allow it, too? I just want to make
sure this change is intentional and desired. Looks like it might have
been in there to support AppleTalk networks (Datagram Delivery Protocol)?
[...]
> + newparam(:protocol) do
> + desc "The protocol the port uses. Valid values are *udp* and *tcp*.
> + Most services have both protocols, but not all. If you want both
> + protocols you have to define two resources. Remeber that you cannot
> + specify two resources with the same title but you can use a title
> + to set both, name and protocol if you use ':' as a seperator. So
> + port { \"telnet/tcp\": ... } sets both name and protocol and you
> don't
> + have to specify them explicitly."
> +
> + newvalues :tcp, :udp
> +
> + defaultto :tcp
> +
> + isnamevar
> + end
Same comment as above about 'ddp'.
[...]
> + it "should have two key_attributes" do
> + @class.key_attributes.size.should == 2
> + end
> +
> + it "should have :name as a key_attribute" do
> + @class.key_attributes.should include :name
> + end
> +
> + it "should have :protocol as a key_attribute" do
> + @class.key_attributes.should include :protocol
> + end
Seems like these three can be condensed down into one test that checks
what key_attributes is, and would be a lot clearer.
Since I haven't actually looked into what key_attributes returns (I
know, shame on me): Does what it return make doing the "this is what it
returns" test prohibitive, instead of the current form of "this is the
shape of what it returns"?
[...]
> + it "should have a list port_aliases" do
> + @class.attrclass(:port_aliases).ancestors.should include
> Puppet::Property::OrderedList
> + end
So, the test description, and the test contents don't seem to line up in
my mind. Also, I'm not sure why we'd care that it's an ancestor of
Puppet::Property::OrderedList in the unit tests, especially given how
it's setup in the implementation.
This feels like an implementation detail that should be tested via
behavior, not explicitly checking ancestry.
[...]
> + it "should not support other protocols than tcp and udp" do
> + proc { @class.new(:name => "whev", :protocol => :tcpp) }.should
> raise_error(Puppet::Error)
> + end
I much prefer the two argument form of .should raise_error(...):
...should raise_error(Puppet::Error, /Aliases must not contain whitespace/)
This way, if we still end up getting a Puppet::Error, but for the wrong
reason, we'll still know about it.
[...]
> + it "should not support portnumbers that arent numeric" do
> + proc { @class.new(:name => "whev", :protocol => :tcp, :number => "aa")
> }.should raise_error(Puppet::Error)
> + proc { @class.new(:name => "whev", :protocol => :tcp, :number =>
> "22a") }.should raise_error(Puppet::Error)
> + proc { @class.new(:name => "whev", :protocol => :tcp, :number =>
> "a22") }.should raise_error(Puppet::Error)
> + end
> +
> + it "should not support portnumbers that are out of range" do
> + proc { @class.new(:name => "whev", :protocol => :tcp, :number => "-1")
> }.should raise_error(Puppet::Error)
> + proc { @class.new(:name => "whev", :protocol => :tcp, :number =>
> "#{2**16}") }.should raise_error(Puppet::Error)
> + end
Same comment about one vs. two argument form of raise_error tests.
[...]
> + it "should not support whitespaces in any port_alias" do
> + proc { @class.new(:name => "whev", :protocol => :tcp, :port_aliases =>
> ['bar','fo o']) }.should raise_error(Puppet::Error)
> + end
> +
> + it "should not support whitespaces in resourcename" do
> + proc { @class.new(:name => "foo bar", :protocol => :tcp) }.should
> raise_error(Puppet::Error)
> + end
> +
> + it "should not allow a resource with no name" do
> + proc { @class.new(:protocol => :tcp) }.should
> raise_error(Puppet::Error)
> + end
> +
> + it "should allow a resource with no protocol when the default is tcp" do
> + proc { @class.new(:name => "foo") }.should_not
> raise_error(Puppet::Error)
> + end
> +
> + it "should not allow a resource with no protocol when we have no
> default" do
> +
> @class.attrclass(:protocol).stubs(:method_defined?).with(:default).returns(false)
> + proc { @class.new(:name => "foo") }.should raise_error(Puppet::Error)
> + end
raise_error, again.
[...]
> + describe "when syncing" do
> +
> + it "should send the first value to the provider for number property" do
> + number = @class.attrclass(:number).new(:resource => @resource, :should
> => %w{100 200})
> + @provider.expects(:number=).with '100'
> + number.sync
> + end
This is purely my own ignorance, but: Why is this particular
test/behavior important?
It looks like it's testing a manifest that looks like
port { 'foo/tcp':
number => ['100', '200'],
}
and expecting it to only act on '100'?
Am I understanding this? (I'm assuming that I am.)
[...]
> + describe "when adding resource to a catalog" do
> +
> + it "should not allow two resources with the same name and protocol" do
> + res1 = @class.new(:name => "telnet", :protocol => :tcp, :number =>
> '23')
> + res2 = @class.new(:name => "telnet", :protocol => :tcp, :number =>
> '23')
> + proc { @catalog.add_resource(res1) }.should_not raise_error
> + proc { @catalog.add_resource(res2) }.should
> raise_error(Puppet::Resource::Catalog::DuplicateResourceError)
> + end
> +
> + it "should allow two resources with different name and protocol" do
> + res1 = @class.new(:name => "telnet", :protocol => :tcp, :number =>
> '23')
> + res2 = @class.new(:name => "git", :protocol => :tcp, :number =>
> '9418')
> + proc { @catalog.add_resource(res1) }.should_not raise_error
> + proc { @catalog.add_resource(res2) }.should_not raise_error
> + end
> +
> + it "should allow two resources with same name and different protocol" do
> + # I would like to have a gentitle method that would not automatically
> set
> + # title to resource[:name] but to uniqueness_key.join('/') or
> + # similar - stschulte
> + res1 = @class.new(:title => 'telnet/tcp', :name => 'telnet',
> :protocol => :tcp, :number => '23')
> + res2 = @class.new(:title => 'telnet/udp', :name => 'telnet',
> :protocol => :udp, :number => '23')
> + proc { @catalog.add_resource(res1) }.should_not raise_error
> + proc { @catalog.add_resource(res2) }.should_not raise_error
> + end
> +
> + it "should allow two resources with the same protocol but different
> names" do
> + res1 = @class.new(:title => 'telnet/tcp', :name => 'telnet',
> :protocol => :tcp, :number => '23')
> + res2 = @class.new(:title => 'ssh/tcp', :name => 'ssh', :protocol =>
> :tcp, :number => '23')
> + proc { @catalog.add_resource(res1) }.should_not raise_error
> + proc { @catalog.add_resource(res2) }.should_not raise_error
> + end
> +
> + end
Seems like this entire section is covered by the uniqueness_key tests
before it, and that this level of testing is actually testing catalog
behavior, not the type's behavior at all.
[...]
If it seems like I'm nitpicking, it's because I am. ;-) So far this
patch series looks pretty good. I haven't yet gotten to the point of
actually trying to run the tests now that you've rebased the series onto
next. I plan to do that after I've had a chance to read through the
full patch series, but I don't expect it to cause any problems.
Thanks again for putting in the work on this!
--
Jacob Helwig
signature.asc
Description: Digital signature
