From 9ee56f2e67be973da49b1d3f21de1bf87de35e6f Mon Sep 17 00:00:00 2001 From: Markus Roberts Date: Fri, 9 Jul 2010 18:05:04 -0700 Subject: Code smell: Inconsistent indentation and related formatting issues * Replaced 163 occurances of defined\? +([@a-zA-Z_.0-9?=]+) with defined?(\1) This makes detecting subsequent patterns easier. 3 Examples: The code: if ! defined? @parse_config becomes: if ! defined?(@parse_config) The code: return @option_parser if defined? @option_parser becomes: return @option_parser if defined?(@option_parser) The code: if defined? @local and @local becomes: if defined?(@local) and @local * Eliminate trailing spaces. Replaced 428 occurances of ^(.*?) +$ with \1 1 file was skipped. test/ral/providers/host/parsed.rb because 0 * Replace leading tabs with an appropriate number of spaces. Replaced 306 occurances of ^(\t+)(.*) with Tabs are not consistently expanded in all environments. * Don't arbitrarily wrap on sprintf (%) operator. Replaced 143 occurances of (.*['"] *%) +(.*) with Splitting the line does nothing to aid clarity and hinders further refactorings. 3 Examples: The code: raise Puppet::Error, "Cannot create %s: basedir %s is a file" % [dir, File.join(path)] becomes: raise Puppet::Error, "Cannot create %s: basedir %s is a file" % [dir, File.join(path)] The code: Puppet.err "Will not start without authorization file %s" % Puppet[:authconfig] becomes: Puppet.err "Will not start without authorization file %s" % Puppet[:authconfig] The code: $stderr.puts "Could not find host for PID %s with status %s" % [pid, $?.exitstatus] becomes: $stderr.puts "Could not find host for PID %s with status %s" % [pid, $?.exitstatus] * Don't break short arrays/parameter list in two. Replaced 228 occurances of (.*) +(.*) with 3 Examples: The code: puts @format.wrap(type.provider(prov).doc, :indent => 4, :scrub => true) becomes: puts @format.wrap(type.provider(prov).doc, :indent => 4, :scrub => true) The code: assert(FileTest.exists?(daily), "Did not make daily graph for %s" % type) becomes: assert(FileTest.exists?(daily), "Did not make daily graph for %s" % type) The code: assert(prov.target_object(:first).read !~ /^notdisk/, "Did not remove thing from disk") becomes: assert(prov.target_object(:first).read !~ /^notdisk/, "Did not remove thing from disk") * If arguments must wrap, treat them all equally Replaced 510 occurances of lines ending in things like ...(foo, or ...(bar(1,3), with \1 \2 3 Examples: The code: midscope.to_hash(false), becomes: assert_equal( The code: botscope.to_hash(true), becomes: # bottomscope, then checking that we see the right stuff. The code: :path => link, becomes: * Replaced 4516 occurances of ^( *)(.*) with The present code base is supposed to use four-space indentation. In some places we failed to maintain that standard. These should be fixed regardless of the 2 vs. 4 space question. 15 Examples: The code: def run_comp(cmd) puts cmd results = [] old_sync = $stdout.sync $stdout.sync = true line = [] begin open("| #{cmd}", "r") do |f| until f.eof? do c = f.getc becomes: def run_comp(cmd) puts cmd results = [] old_sync = $stdout.sync $stdout.sync = true line = [] begin open("| #{cmd}", "r") do |f| until f.eof? do c = f.getc The code: s.gsub!(/.{4}/n, '\\\\u\&') } string.force_encoding(Encoding::UTF_8) string rescue Iconv::Failure => e raise GeneratorError, "Caught #{e.class}: #{e}" end else def utf8_to_pson(string) # :nodoc: string = string.gsub(/["\\\x0-\x1f]/) { MAP[$&] } string.gsub!(/( becomes: s.gsub!(/.{4}/n, '\\\\u\&') } string.force_encoding(Encoding::UTF_8) string rescue Iconv::Failure => e raise GeneratorError, "Caught #{e.class}: #{e}" end else def utf8_to_pson(string) # :nodoc: string = string.gsub(/["\\\x0-\x1f]/) { MAP[$&] } string.gsub!(/( The code: end } rvalues: rvalue | rvalues comma rvalue { if val[0].instance_of?(AST::ASTArray) result = val[0].push(val[2]) else result = ast AST::ASTArray, :children => [val[0],val[2]] end } becomes: end } rvalues: rvalue | rvalues comma rvalue { if val[0].instance_of?(AST::ASTArray) result = val[0].push(val[2]) else result = ast AST::ASTArray, :children => [val[0],val[2]] end } The code: #passwdproc = proc { @password } keytext = @key.export( OpenSSL::Cipher::DES.new(:EDE3, :CBC), @password ) File.open(@keyfile, "w", 0400) { |f| f << keytext } becomes: # passwdproc = proc { @password } keytext = @key.export( OpenSSL::Cipher::DES.new(:EDE3, :CBC), @password ) File.open(@keyfile, "w", 0400) { |f| f << keytext } The code: end def to_manifest "%s { '%s':\n%s\n}" % [self.type.to_s, self.name, @params.collect { |p, v| if v.is_a? Array " #{p} => [\'#{v.join("','")}\']" else " #{p} => \'#{v}\'" end }.join(",\n") becomes: end def to_manifest "%s { '%s':\n%s\n}" % [self.type.to_s, self.name, @params.collect { |p, v| if v.is_a? Array " #{p} => [\'#{v.join("','")}\']" else " #{p} => \'#{v}\'" end }.join(",\n") The code: via the augeas tool. Requires: - augeas to be installed (http://www.augeas.net) - ruby-augeas bindings Sample usage with a string:: augeas{\"test1\" : context => \"/files/etc/sysconfig/firstboot\", changes => \"set RUN_FIRSTBOOT YES\", becomes: via the augeas tool. Requires: - augeas to be installed (http://www.augeas.net) - ruby-augeas bindings Sample usage with a string:: augeas{\"test1\" : context => \"/files/etc/sysconfig/firstboot\", changes => \"set RUN_FIRSTBOOT YES\", The code: names.should_not be_include("root") end describe "when generating a purgeable resource" do it "should be included in the generated resources" do Puppet::Type.type(:host).stubs(:instances).returns [@purgeable_resource] @resources.generate.collect { |r| r.ref }.should include(@purgeable_resource.ref) end end describe "when the instance's do not have an ensure property" do becomes: names.should_not be_include("root") end describe "when generating a purgeable resource" do it "should be included in the generated resources" do Puppet::Type.type(:host).stubs(:instances).returns [@purgeable_resource] @resources.generate.collect { |r| r.ref }.should include(@purgeable_resource.ref) end end describe "when the instance's do not have an ensure property" do The code: describe "when the instance's do not have an ensure property" do it "should not be included in the generated resources" do @no_ensure_resource = Puppet::Type.type(:exec).new(:name => '/usr/bin/env echo') Puppet::Type.type(:host).stubs(:instances).returns [@no_ensure_resource] @resources.generate.collect { |r| r.ref }.should_not include(@no_ensure_resource.ref) end end describe "when the instance's ensure property does not accept absent" do it "should not be included in the generated resources" do @no_absent_resource = Puppet::Type.type(:service).new(:name => 'foobar') becomes: describe "when the instance's do not have an ensure property" do it "should not be included in the generated resources" do @no_ensure_resource = Puppet::Type.type(:exec).new(:name => '/usr/bin/env echo') Puppet::Type.type(:host).stubs(:instances).returns [@no_ensure_resource] @resources.generate.collect { |r| r.ref }.should_not include(@no_ensure_resource.ref) end end describe "when the instance's ensure property does not accept absent" do it "should not be included in the generated resources" do @no_absent_resource = Puppet::Type.type(:service).new(:name => 'foobar') The code: func = nil assert_nothing_raised do func = Puppet::Parser::AST::Function.new( :name => "template", :ftype => :rvalue, :arguments => AST::ASTArray.new( :children => [stringobj(template)] ) becomes: func = nil assert_nothing_raised do func = Puppet::Parser::AST::Function.new( :name => "template", :ftype => :rvalue, :arguments => AST::ASTArray.new( :children => [stringobj(template)] ) The code: assert( @store.allowed?("hostname.madstop.com", "192.168.1.50"), "hostname not allowed") assert( ! @store.allowed?("name.sub.madstop.com", "192.168.0.50"), "subname name allowed") becomes: assert( @store.allowed?("hostname.madstop.com", "192.168.1.50"), "hostname not allowed") assert( ! @store.allowed?("name.sub.madstop.com", "192.168.0.50"), "subname name allowed") The code: assert_nothing_raised { server = Puppet::Network::Handler.fileserver.new( :Local => true, :Config => false ) } becomes: assert_nothing_raised { server = Puppet::Network::Handler.fileserver.new( :Local => true, :Config => false ) } The code: 'yay', { :failonfail => false, :uid => @user.uid, :gid => @user.gid } ).returns('output') output = Puppet::Util::SUIDManager.run_and_capture 'yay', @user.uid, @user.gid becomes: 'yay', { :failonfail => false, :uid => @user.uid, :gid => @user.gid } ).returns('output') output = Puppet::Util::SUIDManager.run_and_capture 'yay', @user.uid, @user.gid The code: ).times(1) pkg.provider.expects( :aptget ).with( '-y', '-q', 'remove', 'faff' becomes: ).times(1) pkg.provider.expects( :aptget ).with( '-y', '-q', 'remove', 'faff' The code: johnny one two billy three four\n" # Just parse and generate, to make sure it's isomorphic. assert_nothing_raised do assert_equal(text, @parser.to_file(@parser.parse(text)), "parsing was not isomorphic") end end def test_valid_attrs becomes: johnny one two billy three four\n" # Just parse and generate, to make sure it's isomorphic. assert_nothing_raised do assert_equal(text, @parser.to_file(@parser.parse(text)), "parsing was not isomorphic") end end def test_valid_attrs The code: "testing", :onboolean => [true, "An on bool"], :string => ["a string", "A string arg"] ) result = [] should = [] assert_nothing_raised("Add args failed") do @config.addargs(result) end @config.each do |name, element| becomes: "testing", :onboolean => [true, "An on bool"], :string => ["a string", "A string arg"] ) result = [] should = [] assert_nothing_raised("Add args failed") do @config.addargs(result) end @config.each do |name, element| --- test/lib/puppettest.rb | 41 +++++----- test/lib/puppettest/fakes.rb | 3 +- test/lib/puppettest/filetesting.rb | 25 +++--- test/lib/puppettest/parsertesting.rb | 144 ++++++++++++++++++++++----------- test/lib/puppettest/railstesting.rb | 7 +- test/lib/puppettest/resourcetesting.rb | 10 ++- test/lib/puppettest/servertest.rb | 7 +- test/lib/puppettest/support/utils.rb | 2 +- 8 files changed, 152 insertions(+), 87 deletions(-) (limited to 'test/lib') diff --git a/test/lib/puppettest.rb b/test/lib/puppettest.rb index f94fb4cec..e11de04e2 100755 --- a/test/lib/puppettest.rb +++ b/test/lib/puppettest.rb @@ -52,11 +52,14 @@ module PuppetTest def self.munge_argv require 'getoptlong' - result = GetoptLong.new( - [ "--debug", "-d", GetoptLong::NO_ARGUMENT ], - [ "--resolve", "-r", GetoptLong::REQUIRED_ARGUMENT ], - [ "-n", GetoptLong::REQUIRED_ARGUMENT ], - [ "--help", "-h", GetoptLong::NO_ARGUMENT ] + + result = GetoptLong.new( + + [ "--debug", "-d", GetoptLong::NO_ARGUMENT ], + [ "--resolve", "-r", GetoptLong::REQUIRED_ARGUMENT ], + [ "-n", GetoptLong::REQUIRED_ARGUMENT ], + + [ "--help", "-h", GetoptLong::NO_ARGUMENT ] ) usage = "USAGE: TESTOPTS='[-n -n ...] [-d]' rake [target] [target] ..." @@ -93,7 +96,7 @@ module PuppetTest # Find the root of the Puppet tree; this is not the test directory, but # the parent of that dir. def basedir(*list) - unless defined? @@basedir + unless defined?(@@basedir) Dir.chdir(File.dirname(__FILE__)) do @@basedir = File.dirname(File.dirname(Dir.getwd)) end @@ -110,7 +113,7 @@ module PuppetTest end def exampledir(*args) - unless defined? @@exampledir + unless defined?(@@exampledir) @@exampledir = File.join(basedir, "examples") end @@ -170,13 +173,16 @@ module PuppetTest ENV["PATH"] += File::PATH_SEPARATOR + "/usr/sbin" end @memoryatstart = Puppet::Util.memory - if defined? @@testcount + if defined?(@@testcount) @@testcount += 1 else @@testcount = 0 end - @configpath = File.join(tmpdir, + + @configpath = File.join( + tmpdir, + "configdir" + @@testcount.to_s + "/" ) @@ -232,7 +238,7 @@ module PuppetTest end def tempfile - if defined? @@tmpfilenum + if defined?(@@tmpfilenum) @@tmpfilenum += 1 else @@tmpfilenum = 1 @@ -259,13 +265,13 @@ module PuppetTest end def tmpdir - unless defined? @tmpdir and @tmpdir + unless defined?(@tmpdir) and @tmpdir @tmpdir = case Facter["operatingsystem"].value - when "Darwin"; "/private/tmp" - when "SunOS"; "/var/tmp" - else - "/tmp" - end + when "Darwin"; "/private/tmp" + when "SunOS"; "/var/tmp" + else + "/tmp" + end @tmpdir = File.join(@tmpdir, "puppettesting" + Process.pid.to_s) @@ -314,8 +320,7 @@ module PuppetTest diff = @memoryatend - @memoryatstart if diff > 1000 - Puppet.info "%s#%s memory growth (%s to %s): %s" % - [self.class, @method_name, @memoryatstart, @memoryatend, diff] + Puppet.info "%s#%s memory growth (%s to %s): %s" % [self.class, @method_name, @memoryatstart, @memoryatend, diff] end # reset all of the logs diff --git a/test/lib/puppettest/fakes.rb b/test/lib/puppettest/fakes.rb index 30387f606..d421b292f 100644 --- a/test/lib/puppettest/fakes.rb +++ b/test/lib/puppettest/fakes.rb @@ -36,8 +36,7 @@ module PuppetTest def []=(param, value) param = symbolize(param) unless @realresource.valid_parameter?(param) - raise Puppet::DevError, "Invalid attribute %s for %s" % - [param, @realresource.name] + raise Puppet::DevError, "Invalid attribute %s for %s" % [param, @realresource.name] end if @realresource.attrtype(param) == :property @should[param] = value diff --git a/test/lib/puppettest/filetesting.rb b/test/lib/puppettest/filetesting.rb index 1aa1cf703..605de6a73 100644 --- a/test/lib/puppettest/filetesting.rb +++ b/test/lib/puppettest/filetesting.rb @@ -88,9 +88,7 @@ module PuppetTest::FileTesting tolist = file_list(todir).sort fromlist.sort.zip(tolist.sort).each { |a,b| - assert_equal(a, b, - "Fromfile %s with length %s does not match tofile %s with length %s" % - [a, fromlist.length, b, tolist.length]) + assert_equal(a, b, "Fromfile %s with length %s does not match tofile %s with length %s" % [a, fromlist.length, b, tolist.length]) } #assert_equal(fromlist,tolist) @@ -103,18 +101,21 @@ module PuppetTest::FileTesting fromstat = File.stat(fromfile) tostat = File.stat(tofile) [:ftype,:gid,:mode,:uid].each { |method| + assert_equal( - fromstat.send(method), - tostat.send(method) - ) - next if fromstat.ftype == "directory" - if checked < 10 and i % 3 == 0 - from = File.open(fromfile) { |f| f.read } - to = File.open(tofile) { |f| f.read } + fromstat.send(method), + + tostat.send(method) + ) + + next if fromstat.ftype == "directory" + if checked < 10 and i % 3 == 0 + from = File.open(fromfile) { |f| f.read } + to = File.open(tofile) { |f| f.read } - assert_equal(from,to) - checked += 1 + assert_equal(from,to) + checked += 1 end } } diff --git a/test/lib/puppettest/parsertesting.rb b/test/lib/puppettest/parsertesting.rb index 5ca5123e8..f4f0eeb44 100644 --- a/test/lib/puppettest/parsertesting.rb +++ b/test/lib/puppettest/parsertesting.rb @@ -12,7 +12,7 @@ module PuppetTest::ParserTesting attr_writer :evaluate def evaluated? - defined? @evaluated and @evaluated + defined?(@evaluated) and @evaluated end def evaluate(*args) @@ -101,11 +101,14 @@ module PuppetTest::ParserTesting title = stringobj(title) end assert_nothing_raised("Could not create %s %s" % [type, title]) { + return AST::Resource.new( + :file => __FILE__, :line => __LINE__, :title => title, :type => type, + :parameters => resourceinst(params) ) } @@ -119,10 +122,13 @@ module PuppetTest::ParserTesting def resourceoverride(type, title, params) assert_nothing_raised("Could not create %s %s" % [type, name]) { + return AST::ResourceOverride.new( + :file => __FILE__, :line => __LINE__, :object => resourceref(type, title), + :type => type, :parameters => resourceinst(params) ) @@ -131,10 +137,13 @@ module PuppetTest::ParserTesting def resourceref(type, title) assert_nothing_raised("Could not create %s %s" % [type, title]) { + return AST::ResourceReference.new( + :file => __FILE__, :line => __LINE__, :type => type, + :title => stringobj(title) ) } @@ -148,34 +157,46 @@ module PuppetTest::ParserTesting def nameobj(name) assert_nothing_raised("Could not create name %s" % name) { + return AST::Name.new( - :file => tempfile(), - :line => rand(100), - :value => name - ) + + :file => tempfile(), + + :line => rand(100), + :value => name + ) } end def typeobj(name) assert_nothing_raised("Could not create type %s" % name) { + return AST::Type.new( - :file => tempfile(), - :line => rand(100), - :value => name - ) + + :file => tempfile(), + + :line => rand(100), + :value => name + ) } end def nodedef(name) assert_nothing_raised("Could not create node %s" % name) { + return AST::NodeDef.new( + :file => tempfile(), + :line => rand(100), :names => nameobj(name), - :code => AST::ASTArray.new( - :children => [ - varobj("%svar" % name, "%svalue" % name), - fileobj("/%s" % name) + + :code => AST::ASTArray.new( + + :children => [ + varobj("%svar" % name, "%svalue" % name), + + fileobj("/%s" % name) ] ) ) @@ -187,11 +208,14 @@ module PuppetTest::ParserTesting params = hash.collect { |param, value| resourceparam(param, value) } - return AST::ResourceInstance.new( - :file => tempfile(), - :line => rand(100), - :children => params - ) + + return AST::ResourceInstance.new( + + :file => tempfile(), + + :line => rand(100), + :children => params + ) } end @@ -201,21 +225,27 @@ module PuppetTest::ParserTesting value = stringobj(value) end assert_nothing_raised("Could not create param %s" % param) { + return AST::ResourceParam.new( - :file => tempfile(), - :line => rand(100), - :param => param, - :value => value - ) + + :file => tempfile(), + + :line => rand(100), + :param => param, + :value => value + ) } end def stringobj(value) + AST::String.new( - :file => tempfile(), - :line => rand(100), - :value => value - ) + + :file => tempfile(), + + :line => rand(100), + :value => value + ) end def varobj(name, value) @@ -223,54 +253,69 @@ module PuppetTest::ParserTesting value = stringobj(value) end assert_nothing_raised("Could not create %s code" % name) { + return AST::VarDef.new( - :file => tempfile(), - :line => rand(100), - :name => nameobj(name), - :value => value - ) + + :file => tempfile(), + + :line => rand(100), + :name => nameobj(name), + :value => value + ) } end def varref(name) assert_nothing_raised("Could not create %s variable" % name) { + return AST::Variable.new( - :file => __FILE__, - :line => __LINE__, - :value => name - ) + + :file => __FILE__, + :line => __LINE__, + + :value => name + ) } end def argobj(name, value) assert_nothing_raised("Could not create %s compargument" % name) { return AST::CompArgument.new( - :children => [nameobj(name), stringobj(value)] - ) + :children => [nameobj(name), stringobj(value)] + ) } end def defaultobj(type, params) pary = [] params.each { |p,v| + pary << AST::ResourceParam.new( - :file => __FILE__, - :line => __LINE__, - :param => p, - :value => stringobj(v) - ) + + :file => __FILE__, + :line => __LINE__, + :param => p, + + :value => stringobj(v) + ) } - past = AST::ASTArray.new( - :file => __FILE__, - :line => __LINE__, - :children => pary - ) + + past = AST::ASTArray.new( + + :file => __FILE__, + :line => __LINE__, + + :children => pary + ) assert_nothing_raised("Could not create defaults for %s" % type) { + return AST::ResourceDefaults.new( + :file => __FILE__, :line => __LINE__, :type => type, + :parameters => past ) } @@ -283,9 +328,12 @@ module PuppetTest::ParserTesting def functionobj(function, name, ftype = :statement) func = nil assert_nothing_raised do + func = Puppet::Parser::AST::Function.new( + :name => function, :ftype => ftype, + :arguments => AST::ASTArray.new( :children => [nameobj(name)] ) diff --git a/test/lib/puppettest/railstesting.rb b/test/lib/puppettest/railstesting.rb index 04ed0bf09..3af593762 100644 --- a/test/lib/puppettest/railstesting.rb +++ b/test/lib/puppettest/railstesting.rb @@ -32,8 +32,11 @@ module PuppetTest::RailsTesting # Now build a resource resources = [] - resources << mkresource(:type => type, :title => title, :exported => true, - :parameters => params) + + resources << mkresource( + :type => type, :title => title, :exported => true, + + :parameters => params) # Now collect our facts facts = Facter.to_hash diff --git a/test/lib/puppettest/resourcetesting.rb b/test/lib/puppettest/resourcetesting.rb index 2e4db1d76..0949dfff4 100644 --- a/test/lib/puppettest/resourcetesting.rb +++ b/test/lib/puppettest/resourcetesting.rb @@ -4,9 +4,15 @@ module PuppetTest::ResourceTesting def mkevaltest(parser = nil) parser ||= mkparser - @parser.newdefine("evaltest", + + @parser.newdefine( + "evaltest", + :arguments => [%w{one}, ["two", stringobj("755")]], - :code => resourcedef("file", "/tmp", + + :code => resourcedef( + "file", "/tmp", + "owner" => varref("one"), "mode" => varref("two")) ) end diff --git a/test/lib/puppettest/servertest.rb b/test/lib/puppettest/servertest.rb index d658ddba7..bda99c66a 100644 --- a/test/lib/puppettest/servertest.rb +++ b/test/lib/puppettest/servertest.rb @@ -6,7 +6,7 @@ module PuppetTest::ServerTest def setup super - if defined? @@port + if defined?(@@port) @@port += 1 else @@port = 20000 @@ -47,8 +47,11 @@ module PuppetTest::ServerTest # then create the actual server server = nil assert_nothing_raised { - server = Puppet::Network::HTTPServer::WEBrick.new( + + server = Puppet::Network::HTTPServer::WEBrick.new( + :Port => @@port, + :Handlers => handlers ) } diff --git a/test/lib/puppettest/support/utils.rb b/test/lib/puppettest/support/utils.rb index 4d9dd2129..db850d699 100644 --- a/test/lib/puppettest/support/utils.rb +++ b/test/lib/puppettest/support/utils.rb @@ -55,7 +55,7 @@ module PuppetTest::Support::Utils else puts id end - unless defined? @me + unless defined?(@me) raise "Could not retrieve user name; 'id' did not work" end end -- cgit