adding extra checks to make sure networking is secure, and refactoring a heckuva lot of test

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

5 files changed, 298 insertions, 605 deletions

diff --git a/test/server/tc_bucket.rb b/test/server/tc_bucket.rb
index 813ea7a61..264a8da13 100644
--- a/test/server/tc_bucket.rb
+++ b/test/server/tc_bucket.rb
@@ -12,19 +12,94 @@ require 'test/unit'
 require 'puppettest.rb'
 require 'base64'
 
-# $Id$
-class TestBucket < Test::Unit::TestCase
+class TestBucket < ServerTest
+    # run through all of the files and exercise the filebucket methods
+    def checkfiles(client)
+        files = filelist()
+
+        # iterate across all of the files
+        files.each { |file|
+            spin
+            name = File.basename(file)
+            tmppath = File.join(tmpdir,name)
+
+            # 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)
+                }
+            }
+
+            # make sure the copy worked
+            assert(FileTest.exists?(tmppath))
+
+            # backup both the orig file and the tmp file
+            osum = nil
+            tsum = nil
+            nsum = nil
+            spin
+            assert_nothing_raised {
+                osum = client.backup(file)
+            }
+            spin
+            assert_nothing_raised {
+                tsum = client.backup(tmppath)
+            }
+
+            # verify you got the same sum back for both
+            assert(tsum == osum)
+
+            # modify our tmp file
+            File.open(tmppath,File::WRONLY|File::TRUNC) { |wf|
+                wf.print "This is some test text\n"
+            }
+
+            # back it up
+            spin
+            assert_nothing_raised {
+                #STDERR.puts("backing up %s" % tmppath) if $debug
+                nsum = client.backup(tmppath)
+            }
+
+            # and verify the sum changed
+            assert(tsum != nsum)
+
+            # restore the orig
+            spin
+            assert_nothing_raised {
+                nsum = client.restore(tmppath,tsum)
+            }
+
+            # and verify it actually got restored
+            spin
+            contents = File.open(tmppath) { |rf|
+                #STDERR.puts("reading %s" % tmppath) if $debug
+                rf.read
+            }
+            csum = Digest::MD5.hexdigest(contents)
+            assert(tsum == csum)
+        }
+    end
+
+    # a list of files that should be on the system
+    # just something to test moving files around
     def filelist
-        files = []
-            #/etc/passwd /etc/syslog.conf /etc/hosts
+        if defined? @files
+            return @files
+        else
+            @files = []
+        end
+
         %w{
-            who /tmp/bigfile sh uname /etc/passwd /etc/syslog.conf /etc/hosts 
+            who bash vim sh uname /etc/passwd /etc/syslog.conf /etc/hosts 
         }.each { |file|
+            # if it's fully qualified, just add it
             if file =~ /^\//
                 if FileTest.exists?(file)
-                    files.push file
+                    @files.push file
                 end
             else
+                # else if it's unqualified, look for it in our path
                 begin
                     path = %x{which #{file}}
                 rescue => detail
@@ -33,79 +108,66 @@ class TestBucket < Test::Unit::TestCase
                 end
 
                 if path != ""
-                    files.push path.chomp
+                    @files.push path.chomp
                 end
             end
         }
 
-        return files
+        return @files
     end
 
     def setup
-        if __FILE__ == $0
-            Puppet[:loglevel] = :debug
-        end
-
-        @bucket = File::SEPARATOR + File.join("tmp","filebuckettesting")
-
-        @@tmppids = []
-        @@tmpfiles = [@bucket]
-
-        @oldconf = Puppet[:puppetconf]
-        Puppet[:puppetconf] = "/tmp/buckettesting"
-        @oldvar = Puppet[:puppetvar]
-        Puppet[:puppetvar] = "/tmp/buckettesting"
-
-        @@tmpfiles << "/tmp/buckettesting"
-    end
-
-    def teardown
-        @@tmpfiles.each { |file|
-            if FileTest.exists?(file)
-                system("rm -rf %s" % file)
-            end
-        }
-        @@tmppids.each { |pid|
-            system("kill -INT %s" % pid)
-        }
+        super
+        @bucket = File.join(Puppet[:puppetconf], "buckettesting")
 
-        Puppet[:puppetconf] = @oldconf
-        Puppet[:puppetvar] = @oldvar
+        @@tmpfiles << @bucket
     end
 
+    # test operating against the local filebucket object
+    # this calls the direct server methods, which are different than the
+    # Dipper methods
     def test_localserver
         files = filelist()
-        server =nil
+        server = nil
         assert_nothing_raised {
             server = Puppet::Server::FileBucket.new(
                 :Bucket => @bucket
             )
         }
+
+        # iterate across them...
         files.each { |file|
+            spin
             contents = File.open(file) { |of| of.read }
 
             md5 = nil
+
+            # add a file to the repository
             assert_nothing_raised {
                 #STDERR.puts("adding %s" % file) if $debug
                 md5 = server.addfile(Base64.encode64(contents),file)
             }
+
+            # and get it back again
             newcontents = nil
             assert_nothing_raised {
                 #STDERR.puts("getting %s" % file) if $debug
                 newcontents = Base64.decode64(server.getfile(md5))
             }
 
+            # and then make sure they're still the same
             assert(
                 contents == newcontents
             )
         }
     end
 
+    # test with a server and a Dipper
     def test_localboth
         files = filelist()
 
-        tmpdir = File.join(@bucket,"tmpfiledir")
-        Puppet.recmkdir(tmpdir)
+        tmpdir = File.join(tmpdir(),"tmpfiledir")
+        FileUtils.mkdir_p(tmpdir)
 
         bucket = nil
         client = nil
@@ -122,151 +184,38 @@ class TestBucket < Test::Unit::TestCase
                 :Bucket => bucket
             )
         }
-        files.each { |file|
-            name = File.basename(file)
-            tmppath = File.join(tmpdir,name)
-
-            # copy the files to our tmp directory so we can modify them...
-            #STDERR.puts("copying %s" % file) if $debug
-            File.open(tmppath,File::WRONLY|File::TRUNC|File::CREAT) { |wf|
-                File.open(file) { |rf|
-                    wf.print(rf.read)
-                }
-            }
-
-            assert(FileTest.exists?(tmppath))
-
-            osum = nil
-            tsum = nil
-            nsum = nil
-            assert_nothing_raised {
-                #STDERR.puts("backing up %s" % file) if $debug
-                osum = client.backup(file)
-            }
-            assert_nothing_raised {
-                #STDERR.puts("backing up %s" % tmppath) if $debug
-                tsum = client.backup(tmppath)
-            }
-
-            assert(tsum == osum)
 
-            File.open(tmppath,File::WRONLY|File::TRUNC) { |wf|
-                wf.print "This is some test text\n"
-            }
-            assert_nothing_raised {
-                #STDERR.puts("backing up %s" % tmppath) if $debug
-                nsum = client.backup(tmppath)
-            }
-
-            assert(tsum != nsum)
-
-            assert_nothing_raised {
-                #STDERR.puts("restoring %s" % tmppath) if $debug
-                nsum = client.restore(tmppath,tsum)
-            }
+        checkfiles(client)
 
-            contents = File.open(tmppath) { |rf|
-                #STDERR.puts("reading %s" % tmppath) if $debug
-                rf.read
-            }
-            csum = Digest::MD5.hexdigest(contents)
-            assert(tsum == csum)
-        }
     end
 
+    # test that things work over the wire
     def test_webxmlmix
         files = filelist()
 
-        tmpdir = File.join(@bucket,"tmpfiledir")
-        Puppet.recmkdir(tmpdir)
+        tmpdir = File.join(tmpdir(),"tmpfiledir")
+        FileUtils.mkdir_p(tmpdir)
 
-        asign = Puppet[:autosign]
         Puppet[:autosign] = true
-        server = nil
         client = nil
         port = Puppet[:masterport]
-        serverthread = nil
-        pid = fork {
-            server = Puppet::Server.new(
-                :Port => port,
-                :Handlers => {
-                    :CA => {}, # so that certs autogenerate
-                    :FileBucket => {
-                        :Bucket => @bucket,
-                    }
-                }
-            )
-            trap(:INT) { server.shutdown }
-            trap(:TERM) { server.shutdown }
-            server.start
-        }
-        @@tmppids << pid
-        sleep 3
+
+        pid = mkserver(:CA => nil, :FileBucket => { :Bucket => @bucket})
 
         assert_nothing_raised {
             client = Puppet::Client::Dipper.new(
                 :Server => "localhost",
-                :Port => port
+                :Port => @@port
             )
         }
-        files.each { |file|
-            name = File.basename(file)
-            tmppath = File.join(tmpdir,name)
-
-            # copy the files to our tmp directory so we can modify them...
-            #STDERR.puts("copying %s" % file) if $debug
-            File.open(tmppath,File::WRONLY|File::TRUNC|File::CREAT) { |wf|
-                File.open(file) { |rf|
-                    wf.print(rf.read)
-                }
-            }
-
-            assert(FileTest.exists?(tmppath))
-
-            osum = nil
-            tsum = nil
-            nsum = nil
-            assert_nothing_raised {
-                #STDERR.puts("backing up %s" % file) if $debug
-                osum = client.backup(file)
-            }
-            assert_nothing_raised {
-                #STDERR.puts("backing up %s" % tmppath) if $debug
-                tsum = client.backup(tmppath)
-            }
-
-            assert(tsum == osum)
-
-            File.open(tmppath,File::WRONLY|File::TRUNC) { |wf|
-                wf.print "This is some test text\n"
-            }
-            assert_nothing_raised {
-                #STDERR.puts("backing up %s" % tmppath) if $debug
-                nsum = client.backup(tmppath)
-            }
 
-            assert(tsum != nsum)
-
-            assert_nothing_raised {
-                #STDERR.puts("restoring %s" % tmppath) if $debug
-                nsum = client.restore(tmppath,tsum)
-            }
-
-            assert_equal(tsum, nsum)
-
-            contents = File.open(tmppath) { |rf|
-                #STDERR.puts("reading %s" % tmppath) if $debug
-                rf.read
-            }
-            csum = Digest::MD5.hexdigest(contents)
-            assert(tsum == csum)
-        }
+        checkfiles(client)
 
         unless pid
             raise "Uh, we don't have a child pid"
         end
         system("kill %s" % pid)
-
-        Puppet[:autosign] = asign
     end
 end
+
+# $Id$
diff --git a/test/server/tc_ca.rb b/test/server/tc_ca.rb
index de1ed0963..a7050b38d 100644
--- a/test/server/tc_ca.rb
+++ b/test/server/tc_ca.rb
@@ -19,43 +19,23 @@ else
     $short = false
 end
 
-class TestCA < Test::Unit::TestCase
-    def setup
-        if __FILE__ == $0
-            Puppet[:loglevel] = :debug
-            #paths = Puppet::Type.type(:service).searchpath
-            #paths.push "%s/examples/root/etc/init.d" % $puppetbase
-            #Puppet::Type.type(:service).setpath(paths)
-        end
-
-        @@tmpfiles = []
-    end
-
+class TestCA < ServerTest
     def teardown
-        Puppet::Type.allclear
+        super
         print "\n\n" if Puppet[:debug]
-
-        @@tmpfiles.each { |file|
-            if FileTest.exists?(file)
-                system("rm -rf %s" % file)
-            end
-        }
     end
 
-    def test_autocertgeneration
-        ssldir = "/tmp/testcertdir"
-        @@tmpfiles.push ssldir
-        assert_nothing_raised {
-            Puppet[:autosign] = true
-            Puppet[:ssldir] = ssldir
-        }
-        file = File.join($puppetbase, "examples", "code", "head")
+    # verify that we're autosigning
+    def test_zautocertgeneration
+        Puppet[:autosign] = true
         ca = nil
 
+        # create our ca
         assert_nothing_raised {
             ca = Puppet::Server::CA.new()
         }
 
+        # create a cert with a fake name
         key = nil
         csr = nil
         cert = nil
@@ -65,21 +45,26 @@ class TestCA < Test::Unit::TestCase
                 :name => "test.domain.com"
             )
         }
+
+        # make the request
         assert_nothing_raised {
             cert.mkcsr
         }
 
+        # and get it signed
         certtext = nil
         cacerttext = nil
         assert_nothing_raised {
             certtext, cacerttext = ca.getcert(cert.csr.to_s)
         }
 
+        # they should both be strings
         assert_instance_of(String, certtext)
         assert_instance_of(String, cacerttext)
-        x509 = nil
+
+        # and they should both be valid certs
         assert_nothing_raised {
-            x509 = OpenSSL::X509::Certificate.new(certtext)
+            OpenSSL::X509::Certificate.new(certtext)
         }
         assert_nothing_raised {
             OpenSSL::X509::Certificate.new(cacerttext)
@@ -94,23 +79,25 @@ class TestCA < Test::Unit::TestCase
         assert_equal(certtext,newtext)
     end
 
+    # this time don't use autosign
     def test_storeAndSign
-        ssldir = "/tmp/testcertdir"
-        @@tmpfiles.push ssldir
         assert_nothing_raised {
-            Puppet[:ssldir] = ssldir
             Puppet[:autosign] = false
         }
-        file = File.join($puppetbase, "examples", "code", "head")
         ca = nil
         caserv = nil
+
+        # make our CA server
         assert_nothing_raised {
             caserv = Puppet::Server::CA.new()
         }
+
+        # retrieve the actual ca object
         assert_nothing_raised {
             ca = caserv.ca
         }
 
+        # make our test cert again
         key = nil
         csr = nil
         cert = nil
@@ -120,21 +107,27 @@ class TestCA < Test::Unit::TestCase
                 :name => "anothertest.domain.com"
             )
         }
+        # and the CSR
         assert_nothing_raised {
             cert.mkcsr
         }
 
+        # retrieve them
         certtext = nil
         assert_nothing_raised {
             certtext, cacerttext = caserv.getcert(cert.csr.to_s)
         }
 
+        # verify we got nothing back, since autosign is off
         assert_equal("", certtext)
 
+        # now sign it manually, with the CA object
         x509 = nil
         assert_nothing_raised {
             x509, cacert = ca.sign(cert.csr)
         }
+
+        # and write it out
         cert.cert = x509
         assert_nothing_raised {
             cert.write
@@ -142,65 +135,22 @@ class TestCA < Test::Unit::TestCase
 
         assert(File.exists?(cert.certfile))
 
+        # now get them again, and verify that we actually get them
         newtext = nil
         assert_nothing_raised {
             newtext, cacerttext  = caserv.getcert(cert.csr.to_s)
         }
 
         assert(newtext)
-    end
-
-    def cycleautosign
-        ssldir = "/tmp/testcertdir"
-        autosign = "/tmp/autosign"
-        @@tmpfiles.push ssldir
-        @@tmpfiles.push autosign
-        assert_nothing_raised {
-            Puppet[:ssldir] = ssldir
-        }
-        file = File.join($puppetbase, "examples", "code", "head")
-        caserv = nil
-
-        assert_nothing_raised {
-            caserv = Puppet::Server::CA.new()
-        }
-
-        key = nil
-        csr = nil
-        cert = nil
-        hostname = "test.domain.com"
-        assert_nothing_raised {
-            cert = Puppet::SSLCertificates::Certificate.new(
-                :name => "test.domain.com"
-            )
-        }
-        assert_nothing_raised {
-            cert.mkcsr
-        }
-
-        certtext = nil
         assert_nothing_raised {
-            certtext = caserv.getcert(cert.csr.to_s)
+            OpenSSL::X509::Certificate.new(newtext)
         }
-
-        x509 = nil
-        assert_nothing_raised {
-            x509 = OpenSSL::X509::Certificate.new(certtext)
-        }
-
-        assert(File.exists?(cert.certfile))
-
-        newtext = nil
-        assert_nothing_raised {
-            newtext = caserv.getcert(cert.csr.to_s)
-        }
-
-        assert_equal(certtext,newtext)
     end
 
+    # and now test the autosign file
     def test_autosign
-        autosign = "/tmp/autosign"
-        Puppet[:autosign] = "/tmp/autosign"
+        autosign = File.join(tmpdir, "autosigntesting")
+        Puppet[:autosign] = autosign
         @@tmpfiles << autosign
         File.open(autosign, "w") { |f|
             f.puts "hostmatch.domain.com"
@@ -208,11 +158,11 @@ class TestCA < Test::Unit::TestCase
         }
 
         caserv = nil
-        file = File.join($puppetbase, "examples", "code", "head")
         assert_nothing_raised {
             caserv = Puppet::Server::CA.new()
         }
 
+        # make sure we know what's going on
         assert(caserv.autosign?("hostmatch.domain.com"))
         assert(caserv.autosign?("fakehost.other.com"))
         assert(!caserv.autosign?("kirby.reductivelabs.com"))
diff --git a/test/server/tc_fileserver.rb b/test/server/tc_fileserver.rb
index 39c286ccd..4235a725c 100755
--- a/test/server/tc_fileserver.rb
+++ b/test/server/tc_fileserver.rb
@@ -14,22 +14,25 @@ require 'test/unit'
 require 'puppettest.rb'
 
 class TestFileServer < TestPuppet
-    def setup
-        if __FILE__ == $0
-            Puppet[:loglevel] = :debug
-        end
-
-        @@tmppids = []
-        super
-    end
+    # make a simple file source
+    def mktestdir
+        testdir = File.join(tmpdir(), "remotefilecopytesting")
+        @@tmpfiles << testdir
 
-    def teardown
-        super
-        @@tmppids.each { |pid|
-            system("kill -INT %s" % pid)
+        # create a tmpfile
+        pattern = "tmpfile"
+        tmpfile = File.join(testdir, pattern)
+        assert_nothing_raised {
+            Dir.mkdir(testdir)
+            File.open(tmpfile, "w") { |f|
+                3.times { f.puts rand(100) }
+            }
         }
+
+        return [testdir, %r{#{pattern}}, tmpfile]
     end
 
+    # make a bunch of random test files
     def mktestfiles(testdir)
         @@tmpfiles << testdir
         assert_nothing_raised {
@@ -48,6 +51,17 @@ class TestFileServer < TestPuppet
         }
     end
 
+    def assert_describe(base, file, server)
+        file = File.basename(file)
+        assert_nothing_raised {
+            desc = server.describe(base + file)
+            assert(desc, "Got no description for %s" % file)
+            assert(desc != "", "Got no description for %s" % file)
+            assert_match(/^\d+/, desc, "Got invalid description %s" % desc)
+        }
+    end
+
+    # test for invalid names
     def test_namefailures
         server = nil
         assert_nothing_raised {
@@ -74,21 +88,15 @@ class TestFileServer < TestPuppet
         }
     end
 
+    # verify that listing the root behaves as expected
     def test_listroot
         server = nil
-        testdir = "/tmp/remotefilecopying"
-        tmpfile = File.join(testdir, "tmpfile")
-        assert_nothing_raised {
-            Dir.mkdir(testdir)
-            File.open(tmpfile, "w") { |f|
-                3.times { f.puts rand(100) }
-            }
-            @@tmpfiles << testdir
-        }
+        testdir, pattern, tmpfile = mktestdir()
 
         file = nil
         checks = Puppet::Server::FileServer::CHECKPARAMS
 
+        # and make our fileserver
         assert_nothing_raised {
             server = Puppet::Server::FileServer.new(
                 :Local => true,
@@ -96,39 +104,30 @@ class TestFileServer < TestPuppet
             )
         }
 
+        # mount the testdir
         assert_nothing_raised {
             server.mount(testdir, "test")
         }
 
+        # and verify different iterations of 'root' return the same value
         list = nil
         assert_nothing_raised {
             list = server.list("/test/", true, false)
         }
 
-        assert(list =~ /tmpfile/)
+        assert(list =~ pattern)
 
         assert_nothing_raised {
             list = server.list("/test", true, false)
         }
-        assert(list =~ /tmpfile/)
+        assert(list =~ pattern)
 
     end
 
+    # test listing individual files
     def test_getfilelist
         server = nil
-        testdir = "/tmp/remotefilecopying"
-        #subdir = "testingyo"
-        #subpath = File.join(testdir, "testingyo")
-        #dir = File.join(testdir, subdir)
-        tmpfile = File.join(testdir, "tmpfile")
-        assert_nothing_raised {
-            Dir.mkdir(testdir)
-            #Dir.mkdir(subpath)
-            File.open(tmpfile, "w") { |f|
-                3.times { f.puts rand(100) }
-            }
-            @@tmpfiles << testdir
-        }
+        testdir, pattern, tmpfile = mktestdir()
 
         file = nil
 
@@ -143,6 +142,7 @@ class TestFileServer < TestPuppet
             server.mount(testdir, "test")
         }
 
+        # get our listing
         list = nil
         sfile = "/test/tmpfile"
         assert_nothing_raised {
@@ -155,12 +155,18 @@ class TestFileServer < TestPuppet
 
         output = "/\tfile"
 
+        # verify it got listed as a file
         assert_equal(output, list)
+
+        # verify we got all fields
         assert(list !~ /\t\t/)
 
+        # verify that we didn't get the directory itself
         list.split("\n").each { |line|
             assert(line !~ %r{remotefile})
         }
+
+        # and then verify that the contents match
         contents = File.read(tmpfile)
 
         ret = nil
@@ -171,19 +177,15 @@ class TestFileServer < TestPuppet
         assert_equal(contents, ret)
     end
 
+    # check that the fileserver is seeing newly created files
     def test_seenewfiles
         server = nil
-        testdir = "/tmp/remotefilecopying"
-        oldfile = File.join(testdir, "oldfile")
+        testdir, pattern, tmpfile = mktestdir()
+
+
         newfile = File.join(testdir, "newfile")
-        assert_nothing_raised {
-            Dir.mkdir(testdir)
-            File.open(oldfile, "w") { |f|
-                3.times { f.puts rand(100) }
-            }
-            @@tmpfiles << testdir
-        }
 
+        # go through the whole schtick again...
         file = nil
         checks = Puppet::Server::FileServer::CHECKPARAMS
 
@@ -204,6 +206,7 @@ class TestFileServer < TestPuppet
             list = server.list(sfile, true, false)
         }
 
+        # create the new file
         File.open(newfile, "w") { |f|
             3.times { f.puts rand(100) }
         }
@@ -213,11 +216,15 @@ class TestFileServer < TestPuppet
             newlist = server.list(sfile, true, false)
         }
 
+        # verify the list has changed
         assert(list != newlist)
 
+        # and verify that we are specifically seeing the new file
         assert(newlist =~ /newfile/)
     end
 
+    # verify we can mount /, which is what local file servers will
+    # normally do
     def test_mountroot
         server = nil
         assert_nothing_raised {
@@ -231,24 +238,17 @@ class TestFileServer < TestPuppet
             server.mount("/", "root")
         }
 
-        testdir = "/tmp/remotefilecopying"
-        oldfile = File.join(testdir, "oldfile")
-        assert_nothing_raised {
-            Dir.mkdir(testdir)
-            File.open(oldfile, "w") { |f|
-                3.times { f.puts rand(100) }
-            }
-            @@tmpfiles << testdir
-        }
+        testdir, pattern, tmpfile = mktestdir()
 
         list = nil
         assert_nothing_raised {
             list = server.list("/root/" + testdir, true, false)
         }
 
-        assert(list =~ /oldfile/)
+        assert(list =~ pattern)
     end
 
+    # verify that we're correctly recursing the right number of levels
     def test_recursionlevels
         server = nil
         assert_nothing_raised {
@@ -258,7 +258,8 @@ class TestFileServer < TestPuppet
             )
         }
 
-        basedir = "/tmp/remotefilecopying"
+        # make our deep recursion
+        basedir = File.join(tmpdir(), "recurseremotetesting")
         testdir = "%s/with/some/sub/directories/for/the/purposes/of/testing" % basedir
         oldfile = File.join(testdir, "oldfile")
         assert_nothing_raised {
@@ -273,20 +274,22 @@ class TestFileServer < TestPuppet
             server.mount(basedir, "test")
         }
 
+        # get our list
         list = nil
         assert_nothing_raised {
             list = server.list("/test/with", false, false)
         }
 
+        # make sure we only got one line, since we're not recursing
         assert(list !~ /\n/)
 
+        # for each level of recursion, make sure we get the right list
         [0, 1, 2].each { |num|
             assert_nothing_raised {
                 list = server.list("/test/with", num, false)
             }
 
             count = 0
-            #p list
             while list =~ /\n/
                 list.sub!(/\n/, '')
                 count += 1
@@ -295,6 +298,8 @@ class TestFileServer < TestPuppet
         }
     end
 
+    # verify that we're not seeing the dir we ask for; i.e., that our
+    # list is relative to that dir, not it's parent dir
     def test_listedpath
         server = nil
         assert_nothing_raised {
@@ -304,6 +309,8 @@ class TestFileServer < TestPuppet
             )
         }
 
+
+        # create a deep dir
         basedir = "/tmp/remotefilecopying"
         testdir = "%s/with/some/sub/directories/for/testing" % basedir
         oldfile = File.join(testdir, "oldfile")
@@ -315,11 +322,13 @@ class TestFileServer < TestPuppet
             @@tmpfiles << basedir
         }
 
+        # mounty mounty
         assert_nothing_raised {
             server.mount(basedir, "localhost")
         }
 
         list = nil
+        # and then check a few dirs
         assert_nothing_raised {
             list = server.list("/localhost/with", false, false)
         }
@@ -333,6 +342,7 @@ class TestFileServer < TestPuppet
         assert(list !~ /sub/)
     end
 
+    # test many dirs, not necessarily very deep
     def test_widelists
         server = nil
         assert_nothing_raised {
@@ -366,6 +376,7 @@ class TestFileServer < TestPuppet
         assert_equal(dirs.length + 1, list.length)
     end
 
+    # verify that 'describe' works as advertised
     def test_describe
         server = nil
         testdir = "/tmp/remotefilecopying"
@@ -385,12 +396,14 @@ class TestFileServer < TestPuppet
             server.mount(testdir, "test")
         }
 
+        # get our list
         list = nil
         sfile = "/test/"
         assert_nothing_raised {
             list = server.list(sfile, true, false)
         }
 
+        # and describe each file in the list
         assert_nothing_raised {
             list.split("\n").each { |line|
                 file, type = line.split("\t")
@@ -399,37 +412,32 @@ class TestFileServer < TestPuppet
             }
         }
 
+        # and then make sure we can describe everything that we know is there
         files.each { |file|
-            file = File.basename(file)
-            assert_nothing_raised {
-                desc = server.describe(sfile + file)
-                assert(desc, "Got no description for %s" % file)
-                assert(desc != "", "Got no description for %s" % file)
-                assert_match(/^\d+/, desc, "Got invalid description %s" % desc)
-            }
+            assert_describe(sfile, file, server)
         }
     end
 
+    # test that our config file is parsing and working as planned
     def test_configfile
         server = nil
-        basedir = "/tmp/configfiletesting"
+        basedir = File.join(tmpdir, "fileserverconfigfiletesting")
+        @@tmpfiles << basedir
 
         conftext = "# a test config file\n \n"
 
-        @@tmpfiles << basedir
 
+        # make some dirs for mounting
         Dir.mkdir(basedir)
         mounts = {}
         %w{thing thus these those}.each { |dir|
             path = File.join(basedir, dir)
-            conftext << "[#{dir}]
-    path #{path}
-"
             mounts[dir] = mktestfiles(path)
 
         }
 
-        conffile = "/tmp/fileservertestingfile"
+        # create an example file with each of them
+        conffile = tempfile
         @@tmpfiles << conffile
 
         File.open(conffile, "w") { |f|
@@ -454,6 +462,7 @@ class TestFileServer < TestPuppet
         }
         
 
+        # create a server with the file
         assert_nothing_raised {
             server = Puppet::Server::FileServer.new(
                 :Local => true,
@@ -478,15 +487,12 @@ class TestFileServer < TestPuppet
             }
 
             files.each { |f|
-                file = File.basename(f)
-                desc = server.describe(mount + file)
-                assert(desc, "Got no description for %s" % f)
-                assert(desc != "", "Got no description for %s" % f)
-                assert_match(/^\d+/, desc, "Got invalid description %s" % f)
+                assert_describe(mount, f, server)
             }
         }
 
         # now let's check that things are being correctly forbidden
+        # this is just a map of names and expected results
         {
             "thing" => {
                 :deny => [
@@ -511,6 +517,7 @@ class TestFileServer < TestPuppet
         }.each { |mount, hash|
             mount = "/#{mount}/"
 
+            # run through the map
             hash.each { |type, ary|
                 ary.each { |sub|
                     host, ip = sub
@@ -534,28 +541,27 @@ class TestFileServer < TestPuppet
 
     end
 
+    # verify we reread the config file when it changes
     def test_filereread
         server = nil
-        testdir = "/tmp/filerereadtesting"
 
-        @@tmpfiles << testdir
+        dir = testdir()
 
-        #Dir.mkdir(testdir)
-        files = mktestfiles(testdir)
+        files = mktestfiles(dir)
 
-        conffile = "/tmp/fileservertestingfile"
-        @@tmpfiles << conffile
+        conffile = tempfile()
 
         File.open(conffile, "w") { |f|
             f.print "# a test config file
  
 [thing]
-    path #{testdir}
+    path #{dir}
     allow test1.domain.com
 "
         }
         
 
+        # start our server with a fast timeout
         assert_nothing_raised {
             server = Puppet::Server::FileServer.new(
                 :Local => true,
@@ -579,7 +585,7 @@ class TestFileServer < TestPuppet
             f.print "# a test config file
  
 [thing]
-    path #{testdir}
+    path #{dir}
     allow test2.domain.com
 "
         }
diff --git a/test/server/tc_master.rb b/test/server/tc_master.rb
index 90fd3b3c0..29a10662b 100644
--- a/test/server/tc_master.rb
+++ b/test/server/tc_master.rb
@@ -14,46 +14,22 @@ require 'puppet/client'
 require 'test/unit'
 require 'puppettest.rb'
 
-class TestMaster < Test::Unit::TestCase
-    def setup
-        if __FILE__ == $0
-            Puppet[:loglevel] = :debug
-        end
-
-        @@tmpfiles = []
-    end
-
-    def stopservices
-        if stype = Puppet::Type.type(:service)
-            stype.each { |service|
-                service[:running] = false
-                service.sync
-            }
-        end
-    end
-
+class TestMaster < ServerTest
     def teardown
-        Puppet::Type.allclear
+        super
         print "\n\n\n\n" if Puppet[:debug]
-
-        @@tmpfiles.each { |file|
-            if FileTest.exists?(file)
-                system("rm -rf %s" % file)
-            end
-        }
     end
 
+    # run through all of the existing test files and make sure everything
+    # works
     def test_files
-        Puppet[:debug] = true if __FILE__ == $0
-        Puppet[:puppetconf] = "/tmp/servertestingdir"
-        @@tmpfiles << Puppet[:puppetconf]
+        count = 0
         textfiles { |file|
             Puppet.debug("parsing %s" % file)
-            server = nil
             client = nil
-            threads = []
-            port = 8080
             master = nil
+
+            # create our master
             assert_nothing_raised() {
                 # this is the default server setup
                 master = Puppet::Server::Master.new(
@@ -61,13 +37,15 @@ class TestMaster < Test::Unit::TestCase
                     :Local => true
                 )
             }
+
+            # and our client
             assert_nothing_raised() {
                 client = Puppet::Client::MasterClient.new(
                     :Master => master
                 )
             }
 
-            # pull our configuration
+            # pull our configuration a few times
             assert_nothing_raised() {
                 client.getconfig
                 stopservices
@@ -83,13 +61,15 @@ class TestMaster < Test::Unit::TestCase
                 stopservices
                 Puppet::Type.allclear
             }
+            # only test three files; that's plenty
+            if count > 3
+                break
+            end
+            count += 1
         }
     end
 
     def test_defaultmanifest
-        Puppet[:debug] = true if __FILE__ == $0
-        Puppet[:puppetconf] = "/tmp/servertestingdir"
-        @@tmpfiles << Puppet[:puppetconf]
         textfiles { |file|
             Puppet[:manifest] = file
             client = nil
@@ -118,22 +98,11 @@ class TestMaster < Test::Unit::TestCase
         }
     end
 
-    def test_zfilereread
-        Puppet[:debug] = true if __FILE__ == $0
-        Puppet[:puppetconf] = "/tmp/masterfilereread"
-        Puppet[:puppetvar] = "/tmp/masterfilereread"
-        @@tmpfiles << Puppet[:puppetconf]
+    def test_filereread
+        manifest = mktestmanifest()
 
-        manifest = "/tmp/masterfilerereadmanifest.pp"
-        @@tmpfiles << manifest = "/tmp/masterfilerereadmanifest.pp"
-        file1 = "/tmp/masterfilecreationrearead"
-        file2 = "/tmp/masterfilecreationrearead2"
-        @@tmpfiles << file1
-        @@tmpfiles << file2
+        file2 = @createdfile + "2"
 
-        File.open(manifest, "w") { |f|
-            f.puts %{file { "/tmp/masterfilecreationrearead": create => true } }
-        }
         client = master = nil
         assert_nothing_raised() {
             # this is the default server setup
@@ -154,12 +123,13 @@ class TestMaster < Test::Unit::TestCase
             client.apply
         }
 
-        assert(FileTest.exists?(file1), "First file %s does not exist" % file1)
+        assert(FileTest.exists?(@createdfile),
+            "Created file %s does not exist" % @createdfile)
         sleep 1
         Puppet::Type.allclear
 
         File.open(manifest, "w") { |f|
-            f.puts %{file { "/tmp/masterfilecreationrearead2": create => true } }
+            f.puts "file { \"%s\": create => true }\n" % file2
         }
         assert_nothing_raised {
             client.getconfig
diff --git a/test/server/tc_server.rb b/test/server/tc_server.rb
index 3854f848c..988a1e17a 100644
--- a/test/server/tc_server.rb
+++ b/test/server/tc_server.rb
@@ -21,60 +21,25 @@ else
     $short = false
 end
 
-class TestServer < Test::Unit::TestCase
-    def setup
-        if __FILE__ == $0
-            Puppet[:loglevel] = :debug
-            #paths = Puppet::Type.type(:service).searchpath
-            #paths.push "%s/examples/root/etc/init.d" % $puppetbase
-            #Puppet::Type.type(:service).setpath(paths)
-        end
-
-        @oldconf = Puppet[:puppetconf]
-        Puppet[:puppetconf] = "/tmp/servertesting"
-        @oldvar = Puppet[:puppetvar]
-        Puppet[:puppetvar] = "/tmp/servertesting"
-
-        @@tmpfiles = ["/tmp/servertesting"]
-        @@tmppids = []
-    end
-
-    def stopservices
-        if stype = Puppet::Type.type(:service)
-            stype.each { |service|
-                service[:running] = false
-                service.sync
-            }
-        end
-    end
-
+class TestServer < ServerTest
     def teardown
-        Puppet::Type.allclear
+        super
         print "\n\n\n\n" if Puppet[:debug]
-
-        @@tmpfiles.each { |file|
-            if FileTest.exists?(file)
-                system("rm -rf %s" % file)
-            end
-        }
-        @@tmppids.each { |pid|
-            system("kill -INT %s" % pid)
-        }
-
-        Puppet[:puppetconf] = @oldconf
-        Puppet[:puppetvar] = @oldvar
     end
 
+    # just do a simple start test
     def test_start
-        server = nil
-        Puppet[:ssldir] = "/tmp/serverstarttesting"
         Puppet[:autosign] = true
-        @@tmpfiles << "/tmp/serverstarttesting"
-        port = 8081
-        file = File.join($puppetbase, "examples", "code", "head")
+        server = nil
+        # make a test manifest
+        file = mktestmanifest()
+
+        # create a simple server
+        # we can use threading here because we're not talking to the server,
+        # just starting and stopping it
         assert_nothing_raised() {
             server = Puppet::Server.new(
-                :Port => port,
+                :Port => @@port,
                 :Handlers => {
                     :CA => {}, # so that certs autogenerate
                     :Master => {
@@ -85,6 +50,8 @@ class TestServer < Test::Unit::TestCase
             )
 
         }
+
+        # start it
         sthread = nil
         assert_nothing_raised() {
             trap(:INT) { server.shutdown }
@@ -92,82 +59,29 @@ class TestServer < Test::Unit::TestCase
                 server.start
             }
         }
-        sleep 1
+
+        # and stop it
         assert_nothing_raised {
             server.shutdown
         }
+
+        # and then wait
         assert_nothing_raised {
             sthread.join
         }
     end
 
-    # disabled because i can't find a good way to test client connecting
-    # i'll have to test the external executables
-    def disabled_test_connect_with_threading
-        server = nil
-        Puppet[:ssldir] = "/tmp/serverconnecttesting"
-        Puppet[:autosign] = true
-        @@tmpfiles << "/tmp/serverconnecttesting"
-        threads = []
-        port = 8080
-        server = nil
-        Thread.abort_on_exception = true
-        assert_nothing_raised() {
-            server = Puppet::Server.new(
-                :Port => port,
-                :Handlers => {
-                    :CA => {}, # so that certs autogenerate
-                    :Status => nil
-                }
-            )
-
-        }
-        sthread = Thread.new {
-            assert_nothing_raised() {
-                #trap(:INT) { server.shutdown; Kernel.exit! }
-                trap(:INT) { server.shutdown }
-                server.start
-            }
-        }
-
-        sleep(3)
-        client = nil
-        assert_nothing_raised() {
-            client = XMLRPC::Client.new("localhost", "/RPC2", port, nil, nil,
-                nil, nil, true, 3)
-        }
-        retval = nil
-
-        clthread = Thread.new {
-            assert_nothing_raised() {
-                retval = client.call("status.status", "")
-            }
-        }
-        assert_not_nil(clthread.join(5))
-
-        assert_equal(1, retval)
-        assert_nothing_raised {
-            #system("kill -INT %s" % serverpid)
-            server.shutdown
-        }
-
-        assert_not_nil(sthread.join(5))
-
-        #Process.wait
-    end
-
-    # disabled because i can't find a good way to test client connecting
-    # i'll have to test the external executables
+    # test that we can connect to the server
+    # we have to use fork here, because we apparently can't use threads
+    # to talk to other threads
     def test_connect_with_fork
         server = nil
-        Puppet[:ssldir] = "/tmp/serverconnecttesting"
         Puppet[:autosign] = true
-        @@tmpfiles << "/tmp/serverconnecttesting"
-        serverpid = nil
-        port = 8080
+
+        # create a server just serving status
         assert_nothing_raised() {
             server = Puppet::Server.new(
-                :Port => port,
+                :Port => @@port,
                 :Handlers => {
                     :CA => {}, # so that certs autogenerate
                     :Status => nil
@@ -175,49 +89,45 @@ class TestServer < Test::Unit::TestCase
             )
 
         }
+
+        # and fork
         serverpid = fork {
             assert_nothing_raised() {
-                #trap(:INT) { server.shutdown; Kernel.exit! }
                 trap(:INT) { server.shutdown }
                 server.start
             }
         }
         @@tmppids << serverpid
 
-        sleep(3)
+        # create a status client, and verify it can talk
         client = nil
         assert_nothing_raised() {
-            client = XMLRPC::Client.new("localhost", "/RPC2", port, nil, nil,
-                nil, nil, true, 3)
+            client = Puppet::Client::StatusClient.new(
+                :Server => "localhost",
+                :Port => @@port
+            )
         }
         retval = nil
 
         assert_nothing_raised() {
-            retval = client.call("status.status")
+            retval = client.status
         }
 
         assert_equal(1, retval)
-        #assert_nothing_raised {
-        #    system("kill -INT %s" % serverpid)
-        #    #server.shutdown
-        #}
-
-        #Process.wait
     end
 
-    # disabled because i can't find a good way to test client connecting
-    # i'll have to test the external executables
-    def test_zzgetconfig_with_fork
-        server = nil
-        Puppet[:ssldir] = "/tmp/serverconfigtesting"
+    # similar to the last test, but this time actually run getconfig
+    def test_getconfig_with_fork
         Puppet[:autosign] = true
-        @@tmpfiles << "/tmp/serverconfigtesting"
         serverpid = nil
-        port = 8082
-        file = File.join($puppetbase, "examples", "code", "head")
+
+        file = mktestmanifest()
+
+        server = nil
+        # make our server again
         assert_nothing_raised() {
             server = Puppet::Server.new(
-                :Port => port,
+                :Port => @@port,
                 :Handlers => {
                     :CA => {}, # so that certs autogenerate
                     :Master => {
@@ -237,118 +147,26 @@ class TestServer < Test::Unit::TestCase
         }
         @@tmppids << serverpid
 
-        sleep(3)
         client = nil
 
-        # first use a puppet client object
+        # and then start a masterclient
         assert_nothing_raised() {
             client = Puppet::Client::MasterClient.new(
                 :Server => "localhost",
-                :Port => port
+                :Port => @@port
             )
         }
         retval = nil
 
+        # and run getconfig a couple of times
         assert_nothing_raised() {
             retval = client.getconfig
         }
+        assert_instance_of(Puppet::TransBucket, retval,
+            "Server returned something other than a TransBucket")
 
-        # then use a raw rpc client
         assert_nothing_raised() {
-            client = XMLRPC::Client.new("localhost", "/RPC2", port, nil, nil,
-                nil, nil, true, 3)
-        }
-        retval = nil
-
-        facts = CGI.escape(Marshal.dump(Puppet::Client::MasterClient.facts))
-        assert_nothing_raised() {
-            retval = client.call("puppetmaster.getconfig", facts)
-        }
-
-        #assert_equal(1, retval)
-    end
-
-    # disabled because clients can't seem to connect from in the same process
-    def disabled_test_files
-        Puppet[:debug] = true if __FILE__ == $0
-        Puppet[:puppetconf] = "/tmp/servertestingdir"
-        Puppet[:autosign] = true
-        @@tmpfiles << Puppet[:puppetconf]
-        textfiles { |file|
-            Puppet.debug("parsing %s" % file)
-            server = nil
-            client = nil
-            threads = []
-            port = 8080
-            assert_nothing_raised() {
-                # this is the default server setup
-                server = Puppet::Server.new(
-                    :Port => port,
-                    :Handlers => {
-                        :CA => {}, # so that certs autogenerate
-                        :Master => {
-                            :File => file,
-                        },
-                    }
-                )
-            }
-            assert_nothing_raised() {
-                client = Puppet::Client.new(
-                    :Server => "localhost",
-                    :Port => port
-                )
-            }
-
-            # start the server
-            assert_nothing_raised() {
-                trap(:INT) { server.shutdown }
-                threads << Thread.new {
-                    server.start
-                }
-            }
-
-            # start the client
-            #assert_nothing_raised() {
-            #    threads << Thread.new {
-            #        client.start
-            #    }
-            #}
-
-            sleep(1)
-            # pull our configuration
-            assert_nothing_raised() {
-                client.getconfig
-                stopservices
-                Puppet::Type.allclear
-            }
-            assert_nothing_raised() {
-                client.getconfig
-                stopservices
-                Puppet::Type.allclear
-            }
-            assert_nothing_raised() {
-                client.getconfig
-                stopservices
-                Puppet::Type.allclear
-            }
-
-            # and shut them both down
-            assert_nothing_raised() {
-                [server].each { |thing|
-                    thing.shutdown
-                }
-            }
-
-            # make sure everything's complete before we stop
-            assert_nothing_raised() {
-                threads.each { |thr|
-                    thr.join
-                }
-            }
-            assert_nothing_raised() {
-                stopservices
-            }
-            Puppet::Type.allclear
+            retval = client.getconfig
         }
     end
 end