Ok, I've squashed the following changes into the branch. I believe this
addresses all the issues Jesse and I found in our code review.
Regarding the downstream_from_vertex and upstream_from_vertex, I stuck with
the rewrite Jesse and I suggested because (a) there seemed to be a loose
majority in favor of it, and (b) in my speed tests it proved to be 1.5 times
faster.
diff --git a/lib/puppet/simple_graph.rb b/lib/puppet/simple_graph.rb
index cd80297..6d153b4 100644
--- a/lib/puppet/simple_graph.rb
+++ b/lib/puppet/simple_graph.rb
@@ -33,8 +33,6 @@ class Puppet::SimpleGraph
# Clear our graph.
def clear
- @in_to.each { |k,v| v.clear }
- @out_from.each { |k,v| v.clear }
@in_to.clear
@out_from.clear
@upstream_from.clear
@@ -43,11 +41,11 @@ class Puppet::SimpleGraph
# Which resources depend upon the given resource.
def dependencies(resource)
- vertex?(resource) ? upstream_from_vertex(resource).keys : []
+ vertex?(resource) ? upstream_from_vertex(resource).keys : []
end
def dependents(resource)
- vertex?(resource) ? downstream_from_vertex(resource).keys : []
+ vertex?(resource) ? downstream_from_vertex(resource).keys : []
end
# Whether our graph is directed. Always true. Used to produce dot
files.
@@ -158,8 +156,6 @@ class Puppet::SimpleGraph
# Add a new vertex to the graph.
def add_vertex(vertex)
- @upstream_from.clear
- @downstream_from.clear
@in_to[vertex] ||= {}
@out_from[vertex] ||= {}
end
@@ -200,14 +196,9 @@ class Puppet::SimpleGraph
add_edge Puppet::Relationship.new(source, target, label)
end
- # Find a matching edge. Note that this only finds the first edge,
- # not all of them or whatever.
- def edge(source, target)
- edge?(source,target) && @out_from[source][target].first
- end
-
- def edge_label(source, target)
- (e = edge(source, target)) && e.label
+ # Find all matching edges.
+ def edges_between(source, target)
+ (@out_from[source] || {})[target] || []
end
# Is there an edge between the two vertices?
@@ -315,11 +306,23 @@ class Puppet::SimpleGraph
end
def downstream_from_vertex(v)
- @downstream_from[v] || @out_from[v].keys.inject(@downstream_from[v] =
{}) { |result,node| result.update(node=>1).update
downstream_from_vertex(node) }
+ return @downstream_from[v] if @downstream_from[v]
+ result = @downstream_from[v] = {}
+ @out_from[v].keys.each do |node|
+ result[node] = 1
+ result.update(downstream_from_vertex(node))
+ end
+ result
end
def upstream_from_vertex(v)
- @upstream_from[v] || @in_to[v].keys.inject( @upstream_from[v] =
{}) { |result,node| result.update(node=>1).update upstream_from_vertex(node)
}
+ return @upstream_from[v] if @upstream_from[v]
+ result = @upstream_from[v] = {}
+ @in_to[v].keys.each do |node|
+ result[node] = 1
+ result.update(upstream_from_vertex(node))
+ end
+ result
end
# LAK:FIXME This is just a paste of the GRATR code with slight
modifications.
@@ -363,18 +366,6 @@ class Puppet::SimpleGraph
system('dotty', dotfile)
end
- # Use +dot+ to create a graphical representation of the graph. Returns
the
- # filename of the graphics file.
- def write_to_graphic_file (fmt='png', dotfile='graph')
- src = dotfile + '.dot'
- dot = dotfile + '.' + fmt
-
- File.open(src, 'w') {|f| f << self.to_dot << "\n"}
-
- system( "dot -T#{fmt} #{src} -o #{dot}" )
- dot
- end
-
# Produce the graph files if requested.
def write_graph(name)
return unless Puppet[:graph]
diff --git a/spec/unit/resource/catalog_spec.rb
b/spec/unit/resource/catalog_spec.rb
index f66d330..fbfe29f 100755
--- a/spec/unit/resource/catalog_spec.rb
+++ b/spec/unit/resource/catalog_spec.rb
@@ -374,7 +374,7 @@ describe Puppet::Resource::Catalog, "when compiling" do
@original.add_edge(@r1,@r2)
@original.filter do |r|
r == @r1
- end.edge(@r1,@r2).should_not be
+ end.edge?(@r1,@r2).should_not be
end
end
@@ -933,8 +933,8 @@ describe Puppet::Resource::Catalog, "when converting to
pson" do
@catalog.add_edge(one, two)
@catalog.add_edge(two, three)
- @catalog.edge(one, two ).expects(:to_pson_data_hash).returns
"one_two_pson"
- @catalog.edge(two, three).expects(:to_pson_data_hash).returns
"two_three_pson"
+ @catalog.edges_between(one, two
)[0].expects(:to_pson_data_hash).returns "one_two_pson"
+ @catalog.edges_between(two,
three)[0].expects(:to_pson_data_hash).returns "two_three_pson"
PSON.parse(@catalog.to_pson,:create_additions =>
false)['data']['edges'].sort.should == %w{one_two_pson two_three_pson}.sort
end
diff --git a/spec/unit/simple_graph_spec.rb b/spec/unit/simple_graph_spec.rb
index c5ce50a..fa0bcb0 100755
--- a/spec/unit/simple_graph_spec.rb
+++ b/spec/unit/simple_graph_spec.rb
@@ -111,16 +111,31 @@ describe Puppet::SimpleGraph do
@graph.edge?(:one, :two).should be_true
end
- it "should provide a method for retrieving an edge label" do
- edge = Puppet::Relationship.new(:one, :two, :callback => :awesome)
- @graph.add_edge(edge)
- @graph.edge_label(:one, :two).should == {:callback => :awesome}
- end
+ describe "when retrieving edges between two nodes" do
+ it "should handle the case of nodes not in the graph" do
+ @graph.edges_between(:one, :two).should == []
+ end
- it "should provide a method for retrieving an edge" do
- edge = Puppet::Relationship.new(:one, :two)
- @graph.add_edge(edge)
- @graph.edge(:one, :two).should equal(edge)
+ it "should handle the case of nodes with no edges between them" do
+ @graph.add_vertex(:one)
+ @graph.add_vertex(:two)
+ @graph.edges_between(:one, :two).should == []
+ end
+
+ it "should handle the case of nodes connected by a single edge" do
+ edge = Puppet::Relationship.new(:one, :two)
+ @graph.add_edge(edge)
+ @graph.edges_between(:one, :two).length.should == 1
+ @graph.edges_between(:one, :two)[0].should equal(edge)
+ end
+
+ it "should handle the case of nodes connected by multiple edges" do
+ edge1 = Puppet::Relationship.new(:one, :two, :callback => :foo)
+ edge2 = Puppet::Relationship.new(:one, :two, :callback => :bar)
+ @graph.add_edge(edge1)
+ @graph.add_edge(edge2)
+ Set.new(@graph.edges_between(:one, :two)).should == Set.new([edge1,
edge2])
+ end
end
it "should add the edge source as a vertex if it is not already" do
@@ -247,7 +262,7 @@ describe Puppet::SimpleGraph do
it "should retain labels on edges" do
@graph.add_edge(:one, :two, :callback => :awesome)
- edge = @graph.reversal.edge(:two, :one)
+ edge = @graph.reversal.edges_between(:two, :one)[0]
edge.label.should == {:callback => :awesome}
end
end
@@ -516,14 +531,14 @@ describe Puppet::SimpleGraph do
it "should not add labels to edges that have none" do
@depgraph.add_edge(@two, @three)
splice
- @depgraph.edge_label("c", "i").should == {}
+ @depgraph.edges_between("c", "i")[0].label.should == {}
end
it "should copy labels over edges that have none" do
@depgraph.add_edge("c", @three, {:callback => :refresh})
splice
# And make sure the label got copied.
- @depgraph.edge_label("c", "i").should == {:callback => :refresh}
+ @depgraph.edges_between("c", "i")[0].label.should == {:callback =>
:refresh}
end
it "should not replace a label with a nil label" do
@@ -531,7 +546,7 @@ describe Puppet::SimpleGraph do
@depgraph.add_edge(@middle, @three)
@depgraph.add_edge("c", @three, {:callback => :refresh})
splice
- @depgraph.edge_label("c", "i").should == {:callback => :refresh}
+ @depgraph.edges_between("c", "i")[0].label.should == {:callback =>
:refresh}
end
it "should copy labels to all created edges" do
@@ -541,7 +556,7 @@ describe Puppet::SimpleGraph do
@three.each do |child|
edge = Puppet::Relationship.new("c", child)
@depgraph.should be_edge(edge.source, edge.target)
- @depgraph.edge_label(edge.source, edge.target).should == {:callback
=> :refresh}
+ @depgraph.edges_between(edge.source, edge.target)[0].label.should
== {:callback => :refresh}
end
end
end
diff --git a/spec/unit/util/monkey_patches_spec.rb
b/spec/unit/util/monkey_patches_spec.rb
index b0f61c8..049ed10 100644
--- a/spec/unit/util/monkey_patches_spec.rb
+++ b/spec/unit/util/monkey_patches_spec.rb
@@ -5,3 +5,29 @@ Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f|
File.exist?(f) ? require(f
require 'puppet/util/monkey_patches'
+
+describe "yaml deserialization" do
+ it "should call yaml_initialize when deserializing objects that have that
method defined" do
+ class Puppet::TestYamlInitializeClass
+ attr_reader :foo
+
+ def yaml_initialize(tag, var)
+ var.should == {'foo' => 100}
+ instance_variables.should == []
+ @foo = 200
+ end
+ end
+
+ obj = YAML.load("--- !ruby/object:Puppet::TestYamlInitializeClass\n
foo: 100")
+ obj.foo.should == 200
+ end
+
+ it "should not call yaml_initialize if not defined" do
+ class Puppet::TestYamlNonInitializeClass
+ attr_reader :foo
+ end
+
+ obj = YAML.load("--- !ruby/object:Puppet::TestYamlNonInitializeClass\n
foo: 100")
+ obj.foo.should == 100
+ end
+end
diff --git a/spec/unit/util/zaml_spec.rb b/spec/unit/util/zaml_spec.rb
index 38a9541..2a9bc07 100644
--- a/spec/unit/util/zaml_spec.rb
+++ b/spec/unit/util/zaml_spec.rb
@@ -35,29 +35,3 @@ describe "Pure ruby yaml implementation" do
end
}
end
-
-describe "yaml deserialization" do
- it "should call yaml_initialize when deserializing objects that have that
method defined" do
- class Puppet::TestYamlInitializeClass
- attr_reader :foo
-
- def yaml_initialize(tag, var)
- var.should == {'foo' => 100}
- instance_variable_defined?(:@hook_called).should == false
- @foo = 200
- end
- end
-
- obj = YAML.load("--- !ruby/object:Puppet::TestYamlInitializeClass\n
foo: 100")
- obj.foo.should == 200
- end
-
- it "should not call yaml_initialize if not defined" do
- class Puppet::TestYamlNonInitializeClass
- attr_reader :foo
- end
-
- obj = YAML.load("--- !ruby/object:Puppet::TestYamlNonInitializeClass\n
foo: 100")
- obj.foo.should == 100
- end
-end
--
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.
