Adding some rspec tests for Config.rb, because I am planning on significantly changing its internals and the current tests, I think, will be harder to migrate than just writing rspec tests from scratch.

5 files changed, 345 insertions, 47 deletions

diff --git a/lib/puppet/parser/interpreter.rb b/lib/puppet/parser/interpreter.rb
index 0398115de..291f122a7 100644
--- a/lib/puppet/parser/interpreter.rb
+++ b/lib/puppet/parser/interpreter.rb
@@ -64,7 +64,7 @@ class Puppet::Parser::Interpreter
         begin
             parser = Puppet::Parser::Parser.new(environment)
             if self.code
-                parser.code = self.code
+                parser.string = self.code
             elsif self.file
                 parser.file = self.file
             end
diff --git a/lib/puppet/util/config.rb b/lib/puppet/util/config.rb
index 932314215..9ec777e99 100644
--- a/lib/puppet/util/config.rb
+++ b/lib/puppet/util/config.rb
@@ -11,7 +11,8 @@ class Puppet::Util::Config
 
     @@sync = Sync.new
 
-    attr_reader :file, :timer
+    attr_accessor :file
+    attr_reader :timer
 
     # Retrieve a config value
     def [](param)
@@ -40,16 +41,14 @@ class Puppet::Util::Config
         @@sync.synchronize do # yay, thread-safe
             param = symbolize(param)
             unless @config.include?(param)
-                raise Puppet::Error,
+                raise ArgumentError,
                     "Attempt to assign a value to unknown configuration parameter %s" % param.inspect
             end
             unless @order.include?(param)
                 @order << param
             end
             @config[param].value = value
-            if @returned.include?(param)
-                @returned.delete(param)
-            end
+            @returned.clear
         end
 
         return value
@@ -128,6 +127,15 @@ class Puppet::Util::Config
         @used = []
     end
 
+    # Return a value's description.
+    def description(name)
+        if obj = @config[symbolize(name)]
+            obj.desc
+        else
+            nil
+        end
+    end
+
     def each
         @order.each { |name|
             if @config.include?(name)
@@ -206,6 +214,20 @@ class Puppet::Util::Config
         @returned = {}
     end
 
+    # Return a given object's file metadata.
+    def metadata(param)
+        if obj = @config[symbolize(param)] and obj.is_a?(CFile)
+            return [:owner, :group, :mode].inject({}) do |meta, p|
+                if v = obj.send(p)
+                    meta[p] = v
+                end
+                meta
+            end
+        else
+            nil
+        end
+    end
+
     # Make a directory with the appropriate user, group, and mode
     def mkdir(default)
         obj = nil
@@ -224,13 +246,17 @@ class Puppet::Util::Config
     end
 
     # Return all of the parameters associated with a given section.
-    def params(section)
-        section = section.intern if section.is_a? String
-        @config.find_all { |name, obj|
-            obj.section == section
-        }.collect { |name, obj|
-            name
-        }
+    def params(section = nil)
+        if section
+            section = section.intern if section.is_a? String
+            @config.find_all { |name, obj|
+                obj.section == section
+            }.collect { |name, obj|
+                name
+            }
+        else
+            @config.keys
+        end
     end
 
     # Parse the configuration file.
@@ -490,6 +516,9 @@ class Puppet::Util::Config
         section = symbolize(section)
         defs.each { |name, hash|
             if hash.is_a? Array
+                unless hash.length == 2
+                    raise ArgumentError, "Defaults specified as an array must contain only the default value and the decription"
+                end
                 tmp = hash
                 hash = {}
                 [:default, :desc].zip(tmp).each { |p,v| hash[p] = v }
@@ -499,7 +528,7 @@ class Puppet::Util::Config
             hash[:section] = section
             name = hash[:name]
             if @config.include?(name)
-                raise Puppet::Error, "Parameter %s is already defined" % name
+                raise ArgumentError, "Parameter %s is already defined" % name
             end
             tryconfig = newelement(hash)
             if short = tryconfig.short
@@ -654,6 +683,15 @@ Generated on #{Time.now}.
         @config.has_key?(param)
     end
 
+    def value(param)
+        param = symbolize(param)
+        if obj = @config[param]
+            obj.value
+        else
+            nil
+        end
+    end
+
     # Open a file with the appropriate user, group, and mode
     def write(default, *args)
         obj = nil
@@ -724,30 +762,30 @@ Generated on #{Time.now}.
 
     private
 
-    # Extra extra setting information for files.
+    # Extract extra setting information for files.
     def extract_fileinfo(string)
-        paramregex = %r{(\w+)\s*=\s*([\w\d]+)}
         result = {}
-        string.scan(/\{\s*([^}]+)\s*\}/) do
+        value = string.sub(/\{\s*([^}]+)\s*\}/) do
             params = $1
             params.split(/\s*,\s*/).each do |str|
-                if str =~ /^\s*(\w+)\s*=\s*([\w\w]+)\s*$/
+                if str =~ /^\s*(\w+)\s*=\s*([\w\d]+)\s*$/
                     param, value = $1.intern, $2
                     result[param] = value
                     unless [:owner, :mode, :group].include?(param)
-                        raise Puppet::Error, "Invalid file option '%s'" % param
+                        raise ArgumentError, "Invalid file option '%s'" % param
                     end
 
                     if param == :mode and value !~ /^\d+$/
-                        raise Puppet::Error, "File modes must be numbers"
+                        raise ArgumentError, "File modes must be numbers"
                     end
                 else
-                    raise Puppet::Error, "Could not parse '%s'" % string
+                    raise ArgumentError, "Could not parse '%s'" % string
                 end
             end
-
-            return result
+            ''
         end
+        result[:value] = value.sub(/\s*$/, '')
+        return result
 
         return nil
     end
@@ -769,26 +807,12 @@ Generated on #{Time.now}.
     # support parsing old files with any section, or new files with just two
     # valid sections.
     def parse_file(file)
-        text = nil
-
-        if file.is_a? Puppet::Util::LoadedFile
-            @file = file
-        else
-            @file = Puppet::Util::LoadedFile.new(file)
-        end
+        text = read_file(file)
 
         # Create a timer so that this file will get checked automatically
         # and reparsed if necessary.
         settimer()
 
-        begin
-            text = File.read(@file.file)
-        rescue Errno::ENOENT
-            raise Puppet::Error, "No such file %s" % file
-        rescue Errno::EACCES
-            raise Puppet::Error, "Permission denied to file %s" % file
-        end
-
         result = Hash.new { |names, name|
             names[name] = {}
         }
@@ -801,7 +825,7 @@ Generated on #{Time.now}.
         text.split(/\n/).each { |line|
             count += 1
             case line
-            when /^\[(\w+)\]$/:
+            when /^\s*\[(\w+)\]$/:
                 section = $1.intern # Section names
                 # Add a meta section
                 result[section][:_meta] ||= {}
@@ -821,6 +845,8 @@ Generated on #{Time.now}.
                 # Check to see if this is a file argument and it has extra options
                 begin
                     if value.is_a?(String) and options = extract_fileinfo(value)
+                        value = options[:value]
+                        options.delete(:value)
                         result[section][:_meta][var] = options
                     end
                     result[section][var] = value
@@ -840,6 +866,23 @@ Generated on #{Time.now}.
         return result
     end
 
+    # Read the file in.
+    def read_file(file)
+        if file.is_a? Puppet::Util::LoadedFile
+            @file = file
+        else
+            @file = Puppet::Util::LoadedFile.new(file)
+        end
+
+        begin
+            return File.read(@file.file)
+        rescue Errno::ENOENT
+            raise ArgumentError, "No such file %s" % file
+        rescue Errno::EACCES
+            raise ArgumentError, "Permission denied to file %s" % file
+        end
+    end
+
     # Take all members of a hash and assign their values appropriately.
     def set_parameter_hash(params)
         params.each do |param, value|
@@ -1143,7 +1186,7 @@ Generated on #{Time.now}.
             when true, "true": return true
             when false, "false": return false
             else
-                raise Puppet::Error, "Invalid value '%s' for %s" %
+                raise ArgumentError, "Invalid value '%s' for %s" %
                     [value.inspect, @name]
             end
         end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index d8f326924..aa56fd93e 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -6,7 +6,7 @@ $:.unshift("#{dir}/../test/lib")
 
 require 'mocha'
 require 'spec'
-require 'puppet'
+require 'puppettest'
 
 Spec::Runner.configure do |config|
   config.mock_with :mocha
diff --git a/spec/unit/util/config.rb b/spec/unit/util/config.rb
new file mode 100755
index 000000000..ef0c00262
--- /dev/null
+++ b/spec/unit/util/config.rb
@@ -0,0 +1,249 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+
+describe Puppet::Util::Config, " when specifying defaults" do
+    before do
+        @config = Puppet::Util::Config.new
+    end
+
+    it "should start with no defined parameters" do
+        @config.params.length.should == 0
+    end
+
+    it "should allow specification of default values associated with a section as an array" do
+        @config.setdefaults(:section, :myvalue => ["defaultval", "my description"])
+    end
+
+    it "should fail when a parameter has already been defined" do
+        @config.setdefaults(:section, :myvalue => ["a", "b"])
+        lambda { @config.setdefaults(:section, :myvalue => ["c", "d"]) }.should raise_error(ArgumentError)
+    end
+
+    it "should allow specification of default values associated with a section as a hash" do
+        @config.setdefaults(:section, :myvalue => {:default => "defaultval", :desc => "my description"})
+    end
+
+    it "should consider defined parameters to be valid" do
+        @config.setdefaults(:section, :myvalue => ["defaultval", "my description"])
+        @config.valid?(:myvalue).should be_true
+    end
+
+    it "should require a description when defaults are specified with an array" do
+        lambda { @config.setdefaults(:section, :myvalue => ["a value"]) }.should raise_error(ArgumentError)
+    end
+
+    it "should require a description when defaults are specified with a hash" do
+        lambda { @config.setdefaults(:section, :myvalue => {:default => "a value"}) }.should raise_error(ArgumentError)
+    end
+
+    it "should support specifying owner, group, and mode when specifying files" do
+        @config.setdefaults(:section, :myvalue => {:default => "/some/file", :owner => "blah", :mode => "boo", :group => "yay", :desc => "whatever"})
+    end
+
+    it "should support specifying a short name" do
+        @config.setdefaults(:section, :myvalue => {:default => "w", :desc => "b", :short => "m"})
+    end
+
+    it "should fail when short names conflict" do
+        @config.setdefaults(:section, :myvalue => {:default => "w", :desc => "b", :short => "m"})
+        lambda { @config.setdefaults(:section, :myvalue => {:default => "w", :desc => "b", :short => "m"}) }.should raise_error(ArgumentError)
+    end
+end
+
+describe Puppet::Util::Config, " when setting values" do
+    before do
+        @config = Puppet::Util::Config.new
+        @config.setdefaults :main, :myval => ["val", "desc"]
+        @config.setdefaults :main, :bool => [true, "desc"]
+    end
+
+    it "should provide a method for setting values from other objects" do
+        @config[:myval] = "something else"
+        @config[:myval].should == "something else"
+    end
+
+    it "should support a getopt-specific mechanism for setting values" do
+        @config.handlearg("--myval", "newval")
+        @config[:myval].should == "newval"
+    end
+
+    it "should support a getopt-specific mechanism for turning booleans off" do
+        @config.handlearg("--no-bool")
+        @config[:bool].should == false
+    end
+
+    it "should support a getopt-specific mechanism for turning booleans on" do
+        # Turn it off first
+        @config[:bool] = false
+        @config.handlearg("--bool")
+        @config[:bool].should == true
+    end
+
+    it "should support a mechanism for setting values in a specific search section" do
+        pending "This code requires the search path functionality"
+        #@config.set(:myval, "new value", :cli)
+        #@config[:myval].should == "new value"
+    end
+end
+
+describe Puppet::Util::Config, " when returning values" do
+    before do
+        @config = Puppet::Util::Config.new
+        @config.setdefaults :section, :one => ["ONE", "a"], :two => ["$one TWO", "b"], :three => ["$one $two THREE", "c"]
+    end
+
+    it "should provide a mechanism for returning set values" do
+        @config[:one] = "other"
+        @config[:one].should == "other"
+    end
+
+    it "should return default values if no values have been set" do
+        @config[:one].should == "ONE"
+    end
+
+    it "should support a search path for finding values" do
+        pending "I have no idea how this will work yet"
+    end
+
+    it "should return set values in the order defined in the search path" do
+        pending "Still no clear idea how this will work"
+    end
+
+    it "should interpolate other parameters into returned parameter values" do
+        @config[:one].should == "ONE"
+        @config[:two].should == "ONE TWO"
+        @config[:three].should == "ONE ONE TWO THREE"
+    end
+
+    it "should not cache interpolated values such that stale information is returned" do
+        @config[:two].should == "ONE TWO"
+        @config[:one] = "one"
+        @config[:two].should == "one TWO"
+    end
+end
+
+describe Puppet::Util::Config, " when parsing its configuration" do
+    before do
+        @config = Puppet::Util::Config.new
+        @config.setdefaults :section, :one => ["ONE", "a"], :two => ["$one TWO", "b"], :three => ["$one $two THREE", "c"]
+    end
+
+    it "should not return values outside of its search path" do
+        text = "[main]
+        one = mval
+        [other]
+        two = oval
+        "
+        file = "/some/file"
+        @config.expects(:read_file).with(file).returns(text)
+        @config.parse(file)
+        @config[:one].should == "mval"
+        @config[:two].should == "mval TWO"
+    end
+
+    it "should support an old parse method when per-executable configuration files still exist" do
+        # I'm not going to bother testing this method.
+        @config.should respond_to(:old_parse)
+    end
+
+    it "should convert booleans in the configuration file into Ruby booleans" do
+        text = "[main]
+        one = true
+        two = false
+        "
+        file = "/some/file"
+        @config.expects(:read_file).with(file).returns(text)
+        @config.parse(file)
+        @config[:one].should == true
+        @config[:two].should == false
+    end
+
+    it "should convert integers in the configuration file into Ruby Integers" do
+        text = "[main]
+        one = 65
+        "
+        file = "/some/file"
+        @config.expects(:read_file).with(file).returns(text)
+        @config.parse(file)
+        @config[:one].should == 65
+    end
+
+    it "should support specifying file all metadata (owner, group, mode) in the configuration file" do
+        @config.setdefaults :section, :myfile => ["/my/file", "a"]
+
+        text = "[main]
+        myfile = /other/file {owner = luke, group = luke, mode = 644}
+        "
+        file = "/some/file"
+        @config.expects(:read_file).with(file).returns(text)
+        @config.parse(file)
+        @config[:myfile].should == "/other/file"
+        @config.metadata(:myfile).should == {:owner => "luke", :group => "luke", :mode => "644"}
+    end
+
+    it "should support specifying file a single piece of metadata (owner, group, or mode) in the configuration file" do
+        @config.setdefaults :section, :myfile => ["/my/file", "a"]
+
+        text = "[main]
+        myfile = /other/file {owner = luke}
+        "
+        file = "/some/file"
+        @config.expects(:read_file).with(file).returns(text)
+        @config.parse(file)
+        @config[:myfile].should == "/other/file"
+        @config.metadata(:myfile).should == {:owner => "luke"}
+    end
+end
+
+describe Puppet::Util::Config, " when reparsing its configuration" do
+    before do
+        @config = Puppet::Util::Config.new
+        @config.setdefaults :section, :one => ["ONE", "a"], :two => ["$one TWO", "b"], :three => ["$one $two THREE", "c"]
+    end
+
+    it "should replace in-memory values with on-file values" do
+        # Init the value
+        text = "[main]\none = disk-init\n"
+        file = mock 'file'
+        file.stubs(:changed?).returns(true)
+        file.stubs(:file).returns("/test/file")
+        @config[:one] = "init"
+        @config.file = file
+
+        # Now replace the value
+        text = "[main]\none = disk-replace\n"
+        
+        # This is kinda ridiculous - the reason it parses twice is that
+        # it goes to parse again when we ask for the value, because the
+        # mock always says it should get reparsed.
+        @config.expects(:read_file).with(file).returns(text).times(2)
+        @config.reparse
+        @config[:one].should == "disk-replace"
+    end
+
+    it "should retain parameters set by cli when configuration files are reparsed" do
+        @config.handlearg("--one", "myval")
+        @config[:two] = "otherval"
+    end
+end
+
+describe Puppet::Util::Config, " when being used to manage the host machine" do
+    it "should provide a method that writes files with the correct modes"
+
+    it "should provide a method that creates directories with the correct modes"
+
+    it "should provide a method to declare what directories should exist"
+
+    it "should provide a method to trigger enforcing of file modes on existing files and directories"
+
+    it "should provide a method to convert the file mode enforcement into a Puppet manifest"
+
+    it "should provide an option to create needed users and groups"
+
+    it "should provide a method to print out the current configuration"
+
+    it "should be able to provide all of its parameters in a format compatible with GetOpt::Long"
+
+    it "should not attempt to manage files within /dev"
+end
diff --git a/test/util/config.rb b/test/util/config.rb
index 9a1017058..b75f8e73d 100755
--- a/test/util/config.rb
+++ b/test/util/config.rb
@@ -74,8 +74,9 @@ class TestConfig < Test::Unit::TestCase
         end
 
         trans = nil
+        node = Puppet::Node.new("node")
         assert_nothing_raised do
-            trans = interp.evaluate(nil, {})
+            trans = interp.compile(node)
         end
         assert_nothing_raised("Could not instantiate objects") {
             trans.to_type
@@ -161,11 +162,11 @@ class TestConfig < Test::Unit::TestCase
 
         assert(! @config[:booltest], "Booltest is not false")
 
-        assert_raise(Puppet::Error) {
+        assert_raise(ArgumentError) {
             @config[:booltest] = "yayness"
         }
 
-        assert_raise(Puppet::Error) {
+        assert_raise(ArgumentError) {
             @config[:booltest] = "/some/file"
         }
     end
@@ -204,7 +205,7 @@ class TestConfig < Test::Unit::TestCase
 
     def test_getset
         initial = "an initial value"
-        assert_raise(Puppet::Error) {
+        assert_raise(ArgumentError) {
             @config[:yayness] = initial
         }
 
@@ -413,6 +414,7 @@ yay = /a/path
         )
 
         file = tempfile
+        count = 0
 
         {
             :pass => {
@@ -432,17 +434,19 @@ yay = /a/path
                 %{{owner => you}}
             ]
         }.each do |type, list|
+            count += 1
             list.each do |value|
                 if type == :pass
                     value, should = value[0], value[1]
                 end
+                path = "/other%s" % count
                 # Write our file out
                 File.open(file, "w") do |f|
-                    f.puts %{[main]\nfile = /other%s} % value
+                    f.puts %{[main]\nfile = #{path}#{value}}
                 end
 
                 if type == :fail
-                    assert_raise(Puppet::Error, "Did not fail on %s" % value.inspect) do
+                    assert_raise(ArgumentError, "Did not fail on %s" % value.inspect) do
                         @config.send(:parse_file, file)
                     end
                 else
@@ -451,6 +455,7 @@ yay = /a/path
                         result = @config.send(:parse_file, file)
                     end
                     assert_equal(should, result[:main][:_meta][:file], "Got incorrect return for %s" % value.inspect)
+                    assert_equal(path, result[:main][:file], "Got incorrect value for %s" % value.inspect)
                 end
             end
         end
@@ -490,6 +495,7 @@ yay = /a/path
         # Get the actual object, so we can verify metadata
         file = @config.element(:file)
 
+        assert_equal("/other", file.value, "Did not get correct value")
         assert_equal("you", file.owner, "Did not pass on user")
         assert_equal("you", file.group, "Did not pass on group")
         assert_equal("644", file.mode, "Did not pass on mode")