Fixing a bunch of small bugs, mostly found by testing on solaris, and added a check to the test system that points out memory growth

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@1113 980ebf18-57e1-0310-9a29-db15c13687c0

17 files changed, 139 insertions, 102 deletions

diff --git a/examples/code/snippets/set.pp b/examples/code/snippets/tag.pp
index e6e770dd9..e6e770dd9 100644
--- a/examples/code/snippets/set.pp
+++ b/examples/code/snippets/tag.pp
diff --git a/lib/puppet/client/dipper.rb b/lib/puppet/client/dipper.rb
index 0ad1926ab..bc85236a3 100644
--- a/lib/puppet/client/dipper.rb
+++ b/lib/puppet/client/dipper.rb
@@ -6,6 +6,7 @@ module Puppet
             
             attr_accessor :name
 
+            # Create our bucket client
             def initialize(hash = {})
                 if hash.include?(:Path)
                     bucket = Puppet::Server::FileBucket.new(
@@ -18,26 +19,25 @@ module Puppet
                 super(hash)
             end
 
+            # Back up a file to our bucket
             def backup(file)
                 unless FileTest.exists?(file)
-                    raise(BucketError, "File %s does not exist" % file, caller)
+                    raise(BucketError, "File %s does not exist" % file)
                 end
-                contents = File.open(file) { |of| of.read }
-
+                contents = File.read(file)
                 string = Base64.encode64(contents)
-                #puts "string is created"
 
                 sum = @driver.addfile(string,file)
-                #puts "file %s is added" % file
+                string = ""
+                contents = ""
                 return sum
             end
 
+            # Restore the file
             def restore(file,sum)
                 restore = true
                 if FileTest.exists?(file)
-                    contents = File.open(file) { |of| of.read }
-
-                    cursum = Digest::MD5.hexdigest(contents)
+                    cursum = Digest::MD5.hexdigest(File.read(file))
 
                     # if the checksum has changed...
                     # this might be extra effort
@@ -50,6 +50,7 @@ module Puppet
                     #puts "Restoring %s" % file
                     if tmp = @driver.getfile(sum)
                         newcontents = Base64.decode64(tmp)
+                        tmp = ""
                         newsum = Digest::MD5.hexdigest(newcontents)
                         changed = nil
                         unless FileTest.writable?(file)
@@ -71,7 +72,6 @@ module Puppet
                 else
                     return nil
                 end
-
             end
         end
     end
diff --git a/lib/puppet/parser/ast/tag.rb b/lib/puppet/parser/ast/tag.rb
index ce59558f3..0807d207e 100644
--- a/lib/puppet/parser/ast/tag.rb
+++ b/lib/puppet/parser/ast/tag.rb
@@ -2,7 +2,7 @@ class Puppet::Parser::AST
     # The code associated with a class.  This is different from components
     # in that each class is a singleton -- only one will exist for a given
     # node.
-    class Set < AST::Branch
+    class Tag < AST::Branch
         @name = :class
         attr_accessor :type
 
diff --git a/lib/puppet/server/filebucket.rb b/lib/puppet/server/filebucket.rb
index 704d1c76d..8a4d1a0a4 100755
--- a/lib/puppet/server/filebucket.rb
+++ b/lib/puppet/server/filebucket.rb
@@ -40,8 +40,6 @@ class Server
         end
 
         def initialize(hash)
-            # build our AST
-
             if hash.include?(:ConflictCheck)
                 @conflictchk = hash[:ConflictCheck]
                 hash.delete(:ConflictCheck)
@@ -67,12 +65,8 @@ class Server
 
         # accept a file from a client
         def addfile(string,path, client = nil, clientip = nil)
-            #puts "entering addfile"
             contents = Base64.decode64(string)
-            #puts "string is decoded"
-
             md5 = Digest::MD5.hexdigest(contents)
-            #puts "md5 is made"
 
             bpath, bfile, pathpath = FileBucket.paths(@bucket,md5)
 
@@ -82,11 +76,9 @@ class Server
                 msg += " from #{client}" if client
                 self.info msg
                 # ...then just create the file
-                #puts "creating file"
                 File.open(bfile, File::WRONLY|File::CREAT, 0440) { |of|
                     of.print contents
                 }
-                #puts "File is created"
             else # if the dir already existed...
                 # ...we need to verify that the contents match the existing file
                 if @conflictchk
@@ -95,13 +87,10 @@ class Server
                             "No file at %s for sum %s" % [bfile,md5], caller)
                     end
 
-                    curfile = ""
-                    File.open(bfile) { |of|
-                        curfile = of.read
-                    }
+                    curfile = File.read(bfile)
 
-                    # if the contents don't match, then we've found a conflict
-                    # unlikely, but quite bad
+                    # If the contents don't match, then we've found a conflict.
+                    # Unlikely, but quite bad.
                     if curfile != contents
                         raise(BucketError,
                             "Got passed new contents for sum %s" % md5, caller)
@@ -111,9 +100,10 @@ class Server
                         self.info msg
                     end
                 end
-                #puts "Conflict check is done"
             end
 
+            contents = ""
+
             # in either case, add the passed path to the list of paths
             paths = nil
             addpath = false
@@ -129,14 +119,12 @@ class Server
             else
                 addpath = true
             end
-            #puts "Path is checked"
 
             # if it's a new file, or if our path isn't in the file yet, add it
             if addpath
                 File.open(pathpath, File::WRONLY|File::CREAT|File::APPEND) { |of|
                     of.puts path
                 }
-                #puts "Path is added"
             end
 
             return md5
diff --git a/lib/puppet/sslcertificates/ca.rb b/lib/puppet/sslcertificates/ca.rb
index 1d9823ed1..04d950e21 100644
--- a/lib/puppet/sslcertificates/ca.rb
+++ b/lib/puppet/sslcertificates/ca.rb
@@ -168,8 +168,8 @@ class Puppet::SSLCertificates::CA
 
     # List certificates waiting to be signed.
     def list
-        return Dir.entries(Puppet[:csrdir]).reject { |file|
-            file =~ /^\.+$/
+        return Dir.entries(Puppet[:csrdir]).find_all { |file|
+            file =~ /\.pem$/
         }.collect { |file|
             file.sub(/\.pem$/, '')
         }
diff --git a/lib/puppet/type/package.rb b/lib/puppet/type/package.rb
index c10dd72ec..95bc17f35 100644
--- a/lib/puppet/type/package.rb
+++ b/lib/puppet/type/package.rb
@@ -11,7 +11,7 @@ module Puppet
         @doc = "Manage packages.  There is a basic dichotomy in package
             support right now:  Some package types (e.g., yum and apt) can
             retrieve their own package files, while others (e.g., rpm and
-            sunpkg) cannot.  For those package formats that cannot retrieve
+            sun) cannot.  For those package formats that cannot retrieve
             their own files, you can use the ``source`` parameter to point to
             the correct file.
 
@@ -388,7 +388,7 @@ module Puppet
                 )
             end
             case @platform
-            when "solaris": @default = :sunpkg
+            when "solaris": @default = :sun
             when "gentoo":
                 Puppet.notice "No support for gentoo yet"
                 @default = nil
diff --git a/lib/puppet/type/package/sun.rb b/lib/puppet/type/package/sun.rb
index e58980228..b4b563461 100755
--- a/lib/puppet/type/package/sun.rb
+++ b/lib/puppet/type/package/sun.rb
@@ -1,5 +1,5 @@
 module Puppet
-    Puppet.type(:package).newpkgtype(:sunpkg) do
+    Puppet.type(:package).newpkgtype(:sun) do
         # Get info on a package, optionally specifying a device.
         def info2hash(device = nil)
             names = {
diff --git a/lib/puppet/type/parsedtype/mount.rb b/lib/puppet/type/parsedtype/mount.rb
index e6d233933..69254e1b6 100755
--- a/lib/puppet/type/parsedtype/mount.rb
+++ b/lib/puppet/type/parsedtype/mount.rb
@@ -5,7 +5,6 @@ require 'puppet/type/state'
 
 module Puppet
     newtype(:mount, Puppet::Type::ParsedType) do
-
         ensurable do
             desc "Create, remove, or mount a filesystem mount."
 
@@ -218,7 +217,12 @@ module Puppet
         # Is the mount currently mounted?
         def mounted?
             platform = Facter["operatingsystem"].value
-            %x{df}.split("\n").find do |line|
+            df = "df"
+            case Facter["operatingsystem"].value
+            # Solaris's df prints in a very weird format
+            when "Solaris": df = "df -k"
+            end
+            %x{#{df}}.split("\n").find do |line|
                 fs = line.split(/\s+/)[-1]
                 if platform == "Darwin"
                     fs == "/private/var/automount" + self[:path] or
diff --git a/lib/puppet/type/service/smf.rb b/lib/puppet/type/service/smf.rb
index 3304b0bbc..0f72479ae 100755
--- a/lib/puppet/type/service/smf.rb
+++ b/lib/puppet/type/service/smf.rb
@@ -60,8 +60,9 @@ Puppet.type(:service).newsvctype(:smf) do
         }
 
         if $? != 0
-            raise Puppet::Error,
-                "Could not get status on service %s" % self.name
+            #raise Puppet::Error,
+            warning "Could not get status on service %s" % self.name
+            return :stopped
         end
     end
 
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index 7da32e280..cedb64264 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -36,13 +36,17 @@ module Util
                     gid = self.gid(group)
                 end
 
-                if Process.gid != gid
-                    oldgid = Process.gid
-                    begin
-                        Process.egid = gid
-                    rescue => detail
-                        raise Puppet::Error, "Could not change GID: %s" % detail
+                if gid
+                    if Process.gid != gid
+                        oldgid = Process.gid
+                        begin
+                            Process.egid = gid
+                        rescue => detail
+                            raise Puppet::Error, "Could not change GID: %s" % detail
+                        end
                     end
+                else
+                    Puppet.warning "Could not retrieve GID for %s" % group
                 end
             end
 
@@ -53,14 +57,19 @@ module Util
                     uid = self.uid(user)
                 end
                 uid = self.uid(user)
-                # Now change the uid
-                if Process.uid != uid
-                    olduid = Process.uid
-                    begin
-                        Process.euid = uid
-                    rescue => detail
-                        raise Puppet::Error, "Could not change UID: %s" % detail
+
+                if uid
+                    # Now change the uid
+                    if Process.uid != uid
+                        olduid = Process.uid
+                        begin
+                            Process.euid = uid
+                        rescue => detail
+                            raise Puppet::Error, "Could not change UID: %s" % detail
+                        end
                     end
+                else
+                    Puppet.warning "Could not retrieve UID for %s" % user
                 end
             end
 
@@ -338,6 +347,23 @@ module Util
     end
 
     module_function :benchmark
+
+    def memory
+        unless defined? @pmap
+            pmap = %x{which pmap 2>/dev/null}.chomp
+            if pmap == ""
+                @pmap = nil
+            else
+                @pmap = pmap
+            end
+        end
+        if @pmap
+            return %x{pmap #{Process.pid}| grep total}.chomp.sub(/^\s*total\s+/, '').sub(/K$/, '').to_i
+        else
+            0
+        end
+    end
+    module_function :memory
 end
 end
 
diff --git a/test/executables/puppetca.rb b/test/executables/puppetca.rb
index a38296a54..8a232ab8d 100755
--- a/test/executables/puppetca.rb
+++ b/test/executables/puppetca.rb
@@ -25,7 +25,11 @@ class TestPuppetCA < Test::Unit::TestCase
     end
     
     def runca(args)
-        return %x{puppetca --confdir=#{Puppet[:confdir]} #{args} 2>&1}
+        debug = ""
+        if Puppet[:debug]
+            debug = "-d "
+        end
+        return %x{puppetca --user=#{Puppet[:user]} #{debug} --group=#{Puppet[:group]} --confdir=#{Puppet[:confdir]} #{args} 2>&1}
 
     end
 
@@ -68,6 +72,9 @@ class TestPuppetCA < Test::Unit::TestCase
         signedfile = File.join(Puppet[:signeddir], "host.test.com.pem")
         assert(FileTest.exists?(signedfile), "cert does not exist")
         assert(! FileTest.executable?(signedfile), "cert is executable")
+
+        uid = Puppet::Util.uid(Puppet[:user])
+
         if Process.uid == 0
             assert(! FileTest.owned?(signedfile), "cert is owned by root")
         end
diff --git a/test/language/snippets.rb b/test/language/snippets.rb
index 84ca4d73c..536fab4fd 100755
--- a/test/language/snippets.rb
+++ b/test/language/snippets.rb
@@ -429,7 +429,7 @@ class TestSnippets < Test::Unit::TestCase
 
     # There's no way to actually retrieve the list of classes from the
     # transaction.
-    def snippet_set(trans)
+    def snippet_tag(trans)
         @@tmpfiles << "/tmp/settestingness"
     end
 
diff --git a/test/other/transactions.rb b/test/other/transactions.rb
index c742b682c..34294e6d2 100644
--- a/test/other/transactions.rb
+++ b/test/other/transactions.rb
@@ -33,12 +33,6 @@ class TestTransactions < Test::Unit::TestCase
         end
     end
 
-    def teardown
-        stopservices
-        #print "\n\n" if Puppet[:debug]
-        super
-    end
-
     def newfile(hash = {})
         tmpfile = tempfile()
         File.open(tmpfile, "w") { |f| f.puts rand(100) }
@@ -60,7 +54,6 @@ class TestTransactions < Test::Unit::TestCase
             File.chown(nil, firstgr, tmpfile)
         end
 
-        @@tmpfiles.push tmpfile
         hash[:name] = tmpfile
         assert_nothing_raised() {
             return Puppet.type(:file).create(hash)
@@ -74,7 +67,7 @@ class TestTransactions < Test::Unit::TestCase
                 :type => "init",
                 :path => File.join($puppetbase,"examples/root/etc/init.d"),
                 :hasstatus => true,
-                :check => [:running]
+                :check => [:ensure]
             )
         }
     end
@@ -147,7 +140,7 @@ class TestTransactions < Test::Unit::TestCase
         component = newcomp("service",service)
 
         assert_nothing_raised() {
-            service[:running] = 1
+            service[:ensure] = 1
         }
         service.retrieve
         assert(service.insync?, "Service did not start")
@@ -281,6 +274,7 @@ class TestTransactions < Test::Unit::TestCase
         )
 
         assert_apply(file, svc, exec)
-        assert(FileTest.exists?(newfile), "File did not get created")
+        assert(FileTest.exists?(path), "File did not get created")
+        assert(FileTest.exists?(newfile), "Refresh file did not get created")
     end
 end
diff --git a/test/puppettest.rb b/test/puppettest.rb
index 323da53e6..ffd6d34f1 100644
--- a/test/puppettest.rb
+++ b/test/puppettest.rb
@@ -24,6 +24,7 @@ module TestPuppet
     end
 
     def setup
+        @memoryatstart = Puppet::Util.memory
         if defined? @@testcount
             @@testcount += 1
         else
@@ -99,7 +100,7 @@ module TestPuppet
     def stopservices
         if stype = Puppet::Type.type(:service)
             stype.each { |service|
-                service[:running] = false
+                service[:ensure] = :stopped
                 service.evaluate
             }
         end
@@ -131,6 +132,14 @@ module TestPuppet
         Puppet::Storage.clear
         Puppet.clear
 
+        @memoryatend = Puppet::Util.memory
+        diff = @memoryatend - @memoryatstart
+
+        if diff > 1000
+            Puppet.info "%s#%s memory growth (%s to %s): %s" %
+                [self.class, @method_name, @memoryatstart, @memoryatend, diff]
+        end
+
         # reset all of the logs
         Puppet::Log.close
 
@@ -145,23 +154,24 @@ module TestPuppet
     end
 
     def tempfile
-        if defined? @tmpfilenum
-            @tmpfilenum += 1
+        if defined? @@tmpfilenum
+            @@tmpfilenum += 1
         else
-            @tmpfilenum = 1
+            @@tmpfilenum = 1
         end
-        f = File.join(self.tmpdir(), self.class.to_s + "testfile" + @tmpfilenum.to_s)
+
+        f = File.join(self.tmpdir(), self.class.to_s + "testfile" + @@tmpfilenum.to_s)
         @@tmpfiles << f
         return f
     end
 
     def tstdir
-        if defined? @testdirnum
-            @testdirnum += 1
+        if defined? @@testdirnum
+            @@testdirnum += 1
         else
-            @testdirnum = 1
+            @@testdirnum = 1
         end
-        d = File.join(self.tmpdir(), self.class.to_s + "testdir" + @testdirnum.to_s)
+        d = File.join(self.tmpdir(), self.class.to_s + "testdir" + @@testdirnum.to_s)
         @@tmpfiles << d
         return d
     end
@@ -306,6 +316,10 @@ module TestPuppet
     def filemode(file)
         File.stat(file).mode & 007777
     end
+
+    def memory
+        Puppet::Util.memory
+    end
 end
 
 
diff --git a/test/server/bucket.rb b/test/server/bucket.rb
index d4a2eee37..29d2b4369 100644
--- a/test/server/bucket.rb
+++ b/test/server/bucket.rb
@@ -14,25 +14,36 @@ require 'base64'
 
 class TestBucket < Test::Unit::TestCase
 	include ServerTest
+
+    def out
+        if defined? @num
+            @num += 1
+        else
+            @num = 1
+        end
+
+        Puppet.err "#{Process.pid}: %s: %s" % [@num, memory()]
+        GC.start
+    end
     # run through all of the files and exercise the filebucket methods
     def checkfiles(client)
         files = filelist()
+        #files = %w{/usr/local/bin/vim /etc/motd /etc/motd /etc/motd /etc/motd}
+        #files = %w{/usr/local/bin/vim}
 
         # iterate across all of the files
         files.each { |file|
-            spin
+            Puppet.warning file
+            out
             tempdir = tempfile()
             Dir.mkdir(tempdir)
             name = File.basename(file)
             tmppath = File.join(tempdir,name)
             @@tmpfiles << tmppath
 
+            out
             # copy the files to our tmp directory so we can modify them...
-            File.open(tmppath,File::WRONLY|File::TRUNC|File::CREAT) { |wf|
-                File.open(file) { |rf|
-                    wf.print(rf.read)
-                }
-            }
+            FileUtils.cp(file, tmppath)
 
             # make sure the copy worked
             assert(FileTest.exists?(tmppath))
@@ -41,14 +52,15 @@ class TestBucket < Test::Unit::TestCase
             osum = nil
             tsum = nil
             nsum = nil
-            spin
+            out
             assert_nothing_raised {
                 osum = client.backup(file)
             }
-            spin
+            out
             assert_nothing_raised {
                 tsum = client.backup(tmppath)
             }
+            out
 
             # verify you got the same sum back for both
             assert(tsum == osum)
@@ -57,30 +69,32 @@ class TestBucket < Test::Unit::TestCase
             File.open(tmppath,File::WRONLY|File::TRUNC) { |wf|
                 wf.print "This is some test text\n"
             }
+            out
 
             # back it up
-            spin
             assert_nothing_raised {
                 #STDERR.puts("backing up %s" % tmppath) if $debug
                 nsum = client.backup(tmppath)
             }
+            out
 
             # and verify the sum changed
             assert(tsum != nsum)
 
             # restore the orig
-            spin
             assert_nothing_raised {
                 nsum = client.restore(tmppath,tsum)
             }
+            out
 
             # and verify it actually got restored
-            spin
             contents = File.open(tmppath) { |rf|
                 #STDERR.puts("reading %s" % tmppath) if $debug
                 rf.read
             }
+            out
             csum = Digest::MD5.hexdigest(contents)
+            out
             assert(tsum == csum)
         }
     end
@@ -94,8 +108,9 @@ class TestBucket < Test::Unit::TestCase
             @files = []
         end
 
+            #who bash vim sh uname /etc/passwd /etc/syslog.conf /etc/hosts 
         %w{
-            who bash vim sh uname /etc/passwd /etc/syslog.conf /etc/hosts 
+            vim.ruby
         }.each { |file|
             # if it's fully qualified, just add it
             if file =~ /^\//
@@ -127,6 +142,11 @@ class TestBucket < Test::Unit::TestCase
         @@tmpfiles << @bucket
     end
 
+    #def teardown
+    #    system("lsof -p %s" % Process.pid)
+    #    super
+    #end
+
     # test operating against the local filebucket object
     # this calls the direct server methods, which are different than the
     # Dipper methods
@@ -141,7 +161,6 @@ class TestBucket < Test::Unit::TestCase
 
         # iterate across them...
         files.each { |file|
-            spin
             contents = File.open(file) { |of| of.read }
 
             md5 = nil
@@ -170,10 +189,6 @@ class TestBucket < Test::Unit::TestCase
     def test_localboth
         files = filelist()
 
-        tmpdir = File.join(tmpdir(),"tmpfiledir")
-        @@tmpfiles << tmpdir
-        FileUtils.mkdir_p(tmpdir)
-
         bucket = nil
         client = nil
         threads = []
@@ -190,8 +205,8 @@ class TestBucket < Test::Unit::TestCase
             )
         }
 
+        #4.times { checkfiles(client) }
         checkfiles(client)
-
     end
 
     # test that things work over the wire
diff --git a/test/types/basic.rb b/test/types/basic.rb
index 9b01f0447..1c41d37bc 100644
--- a/test/types/basic.rb
+++ b/test/types/basic.rb
@@ -41,7 +41,7 @@ class TestBasic < Test::Unit::TestCase
                 :type => "init",
                 :path => File.join($puppetbase,"examples/root/etc/init.d"),
                 :hasstatus => true,
-                :running => 1
+                :ensure => :running
             )
         }
         assert_nothing_raised() {
@@ -95,7 +95,7 @@ class TestBasic < Test::Unit::TestCase
             transaction.evaluate
         }
         assert_nothing_raised() {
-            @sleeper[:running] = 0
+            @sleeper[:ensure] = :running
         }
         assert_nothing_raised() {
             transaction = @component.evaluate
diff --git a/test/types/group.rb b/test/types/group.rb
index 372b35cb3..61edeb771 100755
--- a/test/types/group.rb
+++ b/test/types/group.rb
@@ -234,18 +234,6 @@ class TestGroup < Test::Unit::TestCase
         assert_equal(Process.gid, user.is(:gid), "Retrieved UID does not match")
     end
 
-    def test_ensurevals
-        gobj = nil
-        assert_nothing_raised {
-            gobj = Puppet.type(:group).create(
-                :name => name,
-                :ensure => :exists
-            )
-        }
-
-        assert_equal(false, gobj.state(:ensure))
-    end
-
     if Process.uid == 0
         def test_mkgroup
             gobj = nil