Code smell: Line modifiers are preferred to one-line blocks.

* Replaced 6 occurances of (while .*?) *do$ with

  The do is unneeded in the block header form and causes problems
  with the block-to-one-line transformation.

  3 Examples:

      The code:
          while line = f.gets do
      becomes:
          while line = f.gets
      The code:
          while line = shadow.gets do
      becomes:
          while line = shadow.gets
      The code:
          while wrapper = zeros.pop do
      becomes:
          while wrapper = zeros.pop

* Replaced 19 occurances of ((if|unless) .*?) *then$ with

  The then is unneeded in the block header form and causes problems
  with the block-to-one-line transformation.

  3 Examples:

      The code:
          if f = test_files_for(failed).find { |f| failed_trace =~ Regexp.new(f) } then
      becomes:
          if f = test_files_for(failed).find { |f| failed_trace =~ Regexp.new(f) }
      The code:
          unless defined?(@spec_command) then
      becomes:
          unless defined?(@spec_command)
      The code:
          if c == ?\n then
      becomes:
          if c == ?\n

* Replaced 758 occurances of

      ((?:if|unless|while|until) .*)
          (.*)
      end

  with

  The one-line form is preferable provided:

      * The condition is not used to assign a variable
      * The body line is not already modified
      * The resulting line is not too long

  3 Examples:

      The code:
          if Puppet.features.libshadow?
              has_feature :manages_passwords
          end
      becomes:
          has_feature :manages_passwords if Puppet.features.libshadow?
      The code:
          unless (defined?(@current_pool) and @current_pool)
              @current_pool = process_zpool_data(get_pool_data)
          end
      becomes:
          @current_pool = process_zpool_data(get_pool_data) unless (defined?(@current_pool) and @current_pool)
      The code:
          if Puppet[:trace]
              puts detail.backtrace
          end
      becomes:
          puts detail.backtrace if Puppet[:trace]

12 files changed, 21 insertions, 63 deletions

diff --git a/test/ral/providers/cron/crontab.rb b/test/ral/providers/cron/crontab.rb
index 46678dc7d..3dfac96e6 100755
--- a/test/ral/providers/cron/crontab.rb
+++ b/test/ral/providers/cron/crontab.rb
@@ -23,9 +23,7 @@ class TestCronParsedProvider < Test::Unit::TestCase
     # a full cron job.  These tests assume individual record types will always be correctly
     # parsed, so all they
     def sample_crons
-        unless defined?(@sample_crons)
-            @sample_crons = YAML.load(File.read(File.join(@crondir, "crontab_collections.yaml")))
-        end
+        @sample_crons = YAML.load(File.read(File.join(@crondir, "crontab_collections.yaml"))) unless defined?(@sample_crons)
         @sample_crons
     end
 
@@ -33,9 +31,7 @@ class TestCronParsedProvider < Test::Unit::TestCase
     # mapping between records and lines.  We have plenty of redundancy here because
     # we use these records to build up our complex, multi-line cron jobs below.
     def sample_records
-        unless defined?(@sample_records)
-            @sample_records = YAML.load(File.read(File.join(@crondir, "crontab_sample_records.yaml")))
-        end
+        @sample_records = YAML.load(File.read(File.join(@crondir, "crontab_sample_records.yaml"))) unless defined?(@sample_records)
         @sample_records
     end
 
@@ -62,25 +58,19 @@ class TestCronParsedProvider < Test::Unit::TestCase
             assert_equal(value, cron.send(param), "#{param} was not equal in #{msg}")
         end
         %w{command environment minute hour month monthday weekday}.each do |var|
-            unless options.include?(var.intern)
-                assert_equal(:absent, cron.send(var), "#{var} was not parsed absent in #{msg}")
-            end
+            assert_equal(:absent, cron.send(var), "#{var} was not parsed absent in #{msg}") unless options.include?(var.intern)
         end
     end
 
     # Make sure a cron record matches.  This only works for crontab records.
     def assert_record_equal(msg, record, options)
-        unless options.include?(:record_type)
-            raise ArgumentError, "You must pass the required record type"
-        end
+        raise ArgumentError, "You must pass the required record type" unless options.include?(:record_type)
         assert_instance_of(Hash, record, "not an instance of a hash in #{msg}")
         options.each do |param, value|
             assert_equal(value, record[param], "#{param} was not equal in #{msg}")
         end
         FIELDS[record[:record_type]].each do |var|
-            unless options.include?(var)
-                assert_equal(:absent, record[var], "#{var} was not parsed absent in #{msg}")
-            end
+            assert_equal(:absent, record[var], "#{var} was not parsed absent in #{msg}") unless options.include?(var)
         end
     end
 
diff --git a/test/ral/providers/group.rb b/test/ral/providers/group.rb
index 067affd68..d6783bf7f 100755
--- a/test/ral/providers/group.rb
+++ b/test/ral/providers/group.rb
@@ -22,9 +22,7 @@ class TestGroupProvider < Test::Unit::TestCase
     def teardown
         super
         @@tmpgroups.each { |group|
-            unless missing?(group)
-                remove(group)
-            end
+            remove(group) unless missing?(group)
         }
     end
 
diff --git a/test/ral/providers/package.rb b/test/ral/providers/package.rb
index 600770f13..03b81477d 100755
--- a/test/ral/providers/package.rb
+++ b/test/ral/providers/package.rb
@@ -17,9 +17,7 @@ class TestPackageProvider < Test::Unit::TestCase
     def self.load_test_packages
         require 'yaml'
         file = File.join(PuppetTest.datadir(), "providers", "package", "testpackages.yaml")
-        unless FileTest.exists?(file)
-            raise "Could not find file #{file}"
-        end
+        raise "Could not find file #{file}" unless FileTest.exists?(file)
         array = YAML::load(File.read(file)).collect { |hash|
             # Stupid ruby 1.8.1.  YAML is sometimes broken such that
             # symbols end up being strings with the : in them.
@@ -163,9 +161,7 @@ class TestPackageProvider < Test::Unit::TestCase
                     end
                 end
             else
-                if cleancmd
-                    system(cleancmd)
-                end
+                system(cleancmd) if cleancmd
             end
         end
 
diff --git a/test/ral/providers/provider.rb b/test/ral/providers/provider.rb
index f716bc1cf..081020638 100755
--- a/test/ral/providers/provider.rb
+++ b/test/ral/providers/provider.rb
@@ -11,9 +11,7 @@ class TestProvider < Test::Unit::TestCase
     def echo
         echo = Puppet::Util.binary("echo")
 
-        unless echo
-            raise "Could not find 'echo' binary; cannot complete test"
-        end
+        raise "Could not find 'echo' binary; cannot complete test" unless echo
 
         return echo
     end
diff --git a/test/ral/providers/user.rb b/test/ral/providers/user.rb
index 81b3afd3e..e4ec3c125 100755
--- a/test/ral/providers/user.rb
+++ b/test/ral/providers/user.rb
@@ -24,9 +24,7 @@ class TestUserProvider < Test::Unit::TestCase
 
     def teardown
         @@tmpusers.each { |user|
-            unless missing?(user)
-                remove(user)
-            end
+            remove(user) unless missing?(user)
         }
         super
     end
@@ -376,9 +374,7 @@ class TestUserProvider < Test::Unit::TestCase
         Etc.setgrent
         max = 0
         while group = Etc.getgrent
-            if group.gid > max and group.gid < 5000
-                max = group.gid
-            end
+            max = group.gid if group.gid > max and group.gid < 5000
         end
 
         groups = []
diff --git a/test/ral/providers/user/useradd.rb b/test/ral/providers/user/useradd.rb
index 2e98d7f5d..52d933f89 100755
--- a/test/ral/providers/user/useradd.rb
+++ b/test/ral/providers/user/useradd.rb
@@ -64,9 +64,7 @@ class UserAddProviderTest < PuppetTest::TestCase
             end
 
             options = {}
-            while params.length > 0
-                options[params.shift] = params.shift
-            end
+            options[params.shift] = params.shift while params.length > 0
 
             @vals[:groups] = @vals[:groups].join(",")
 
@@ -118,9 +116,7 @@ class UserAddProviderTest < PuppetTest::TestCase
 
         @user.provider.expects(:execute).with do |params|
             assert_equal(params[0], @provider.command(:add), "useradd was not called")
-            if %w{Fedora RedHat}.include?(Facter.value(:operatingsystem))
-                assert(params.include?("-M"), "Did not add -M on Red Hat")
-            end
+            assert(params.include?("-M"), "Did not add -M on Red Hat") if %w{Fedora RedHat}.include?(Facter.value(:operatingsystem))
             assert(! params.include?("-m"), "Added -m when managehome was disabled")
 
             true
@@ -171,9 +167,7 @@ class UserAddProviderTest < PuppetTest::TestCase
     end
 
     def test_manages_password
-        unless @provider.feature?(:manages_passwords)
-            return
-        end
+        return unless @provider.feature?(:manages_passwords)
         @vals[:password] = "somethingorother"
         setup_user
 
diff --git a/test/ral/type/cron.rb b/test/ral/type/cron.rb
index fcee21dd0..6b561c392 100755
--- a/test/ral/type/cron.rb
+++ b/test/ral/type/cron.rb
@@ -197,9 +197,7 @@ class TestCron < Test::Unit::TestCase
                             cron[param] = value
                         }
 
-                        if value.is_a?(Integer)
-                            assert_equal([value.to_s], cron.should(param), "Cron value was not set correctly")
-                        end
+                        assert_equal([value.to_s], cron.should(param), "Cron value was not set correctly") if value.is_a?(Integer)
                     when :invalid
                         assert_raise(Puppet::Error, "#{value} is incorrectly a valid #{param}") {
                             cron[param] = value
diff --git a/test/ral/type/exec.rb b/test/ral/type/exec.rb
index 454458ff3..7ddb4650d 100755
--- a/test/ral/type/exec.rb
+++ b/test/ral/type/exec.rb
@@ -391,9 +391,7 @@ class TestExec < Test::Unit::TestCase
             assert_events([:executed_command], comp, "usertest")
 
             assert(FileTest.exists?(file), "File does not exist")
-            if user
-                assert_equal(user.uid, File.stat(file).uid, "File UIDs do not match")
-            end
+            assert_equal(user.uid, File.stat(file).uid, "File UIDs do not match") if user
 
             # We can't actually test group ownership, unfortunately, because
             # behaviour changes wildlly based on platform.
diff --git a/test/ral/type/filesources.rb b/test/ral/type/filesources.rb
index a928fd7db..7541a7cbe 100755
--- a/test/ral/type/filesources.rb
+++ b/test/ral/type/filesources.rb
@@ -90,9 +90,7 @@ class TestFileSources < Test::Unit::TestCase
 
         todir = File.join(path, "todir")
         source = fromdir
-        if networked
-            source = "puppet://localhost/#{networked}#{fromdir}"
-        end
+        source = "puppet://localhost/#{networked}#{fromdir}" if networked
         recursive_source_test(source, todir)
 
         return [fromdir,todir, File.join(todir, "one"), File.join(todir, "two")]
diff --git a/test/ral/type/resources.rb b/test/ral/type/resources.rb
index eb53a7edf..50d6839e7 100755
--- a/test/ral/type/resources.rb
+++ b/test/ral/type/resources.rb
@@ -24,9 +24,7 @@ class TestResources < Test::Unit::TestCase
         @purgenum += 1
         obj = @purgetype.create :name => "purger#{@purgenum}"
         $purgemembers[obj[:name]] = obj
-        if managed
-            obj[:fake] = "testing"
-        end
+        obj[:fake] = "testing" if managed
         obj
     end
 
@@ -100,9 +98,7 @@ class TestResources < Test::Unit::TestCase
             end
         }
 
-        if low
-            assert(! res.check(@user.create(:name => low)), "low user #{low} passed check")
-        end
+        assert(! res.check(@user.create(:name => low)), "low user #{low} passed check") if low
         if high
             res[:unless_system_user] = 50
             assert(res.check(@user.create(:name => high)), "high user #{high} failed check")
diff --git a/test/ral/type/sshkey.rb b/test/ral/type/sshkey.rb
index e362de839..01d72156a 100755
--- a/test/ral/type/sshkey.rb
+++ b/test/ral/type/sshkey.rb
@@ -32,9 +32,7 @@ class TestSSHKey < Test::Unit::TestCase
 
     def teardown
         super
-        if @provider.respond_to?(:clear)
-            @provider.clear
-        end
+        @provider.clear if @provider.respond_to?(:clear)
     end
 
     def mkkey
diff --git a/test/ral/type/zone.rb b/test/ral/type/zone.rb
index f87774786..c136fbfb1 100755
--- a/test/ral/type/zone.rb
+++ b/test/ral/type/zone.rb
@@ -105,9 +105,7 @@ class TestZone < PuppetTest::TestCase
         assert_nothing_raised {
             property.class.state_sequence(:absent, :running).each do |st|
                 [:up, :down].each do |m|
-                    if st[m]
-                        methods << st[m]
-                    end
+                    methods << st[m] if st[m]
                 end
             end
         }