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/network/handler/ca.rb | 5 +- test/network/handler/fileserver.rb | 256 ++++++++++++++++++++++++------------- 2 files changed, 168 insertions(+), 93 deletions(-) (limited to 'test/network/handler') diff --git a/test/network/handler/ca.rb b/test/network/handler/ca.rb index 503d0188d..4d1531228 100755 --- a/test/network/handler/ca.rb +++ b/test/network/handler/ca.rb @@ -191,8 +191,11 @@ class TestCA < Test::Unit::TestCase server = nil Puppet.stubs(:master?).returns true assert_nothing_raised { - server = Puppet::Network::HTTPServer::WEBrick.new( + + server = Puppet::Network::HTTPServer::WEBrick.new( + :Port => @@port, + :Handlers => { :CA => {}, # so that certs autogenerate :Status => nil diff --git a/test/network/handler/fileserver.rb b/test/network/handler/fileserver.rb index f17d82bf6..357192716 100755 --- a/test/network/handler/fileserver.rb +++ b/test/network/handler/fileserver.rb @@ -72,8 +72,11 @@ class TestFileServer < Test::Unit::TestCase def test_namefailures server = nil assert_nothing_raised { + server = Puppet::Network::Handler.fileserver.new( + :Local => true, + :Config => false ) } @@ -95,8 +98,11 @@ class TestFileServer < Test::Unit::TestCase # and make our fileserver assert_nothing_raised { + server = Puppet::Network::Handler.fileserver.new( + :Local => true, + :Config => false ) } @@ -129,8 +135,11 @@ class TestFileServer < Test::Unit::TestCase file = nil assert_nothing_raised { + server = Puppet::Network::Handler.fileserver.new( + :Local => true, + :Config => false ) } @@ -183,8 +192,11 @@ class TestFileServer < Test::Unit::TestCase checks = Puppet::Network::Handler.fileserver::CHECKPARAMS assert_nothing_raised { + server = Puppet::Network::Handler.fileserver.new( + :Local => true, + :Config => false ) } @@ -221,8 +233,11 @@ class TestFileServer < Test::Unit::TestCase def test_mountroot server = nil assert_nothing_raised { + server = Puppet::Network::Handler.fileserver.new( + :Local => true, + :Config => false ) } @@ -250,8 +265,11 @@ class TestFileServer < Test::Unit::TestCase def test_recursionlevels server = nil assert_nothing_raised { + server = Puppet::Network::Handler.fileserver.new( + :Local => true, + :Config => false ) } @@ -301,8 +319,11 @@ class TestFileServer < Test::Unit::TestCase def test_listedpath server = nil assert_nothing_raised { + server = Puppet::Network::Handler.fileserver.new( + :Local => true, + :Config => false ) } @@ -344,8 +365,11 @@ class TestFileServer < Test::Unit::TestCase def test_widelists server = nil assert_nothing_raised { + server = Puppet::Network::Handler.fileserver.new( + :Local => true, + :Config => false ) } @@ -384,8 +408,11 @@ class TestFileServer < Test::Unit::TestCase checks = Puppet::Network::Handler.fileserver::CHECKPARAMS assert_nothing_raised { + server = Puppet::Network::Handler.fileserver.new( + :Local => true, + :Config => false ) } @@ -425,7 +452,10 @@ class TestFileServer < Test::Unit::TestCase # Now try to describe some sources that don't even exist retval = nil - assert_raise(Puppet::Network::Handler::FileServerError, + + assert_raise( + Puppet::Network::Handler::FileServerError, + "Describing non-existent mount did not raise an error") { retval = server.describe("/notmounted/" + "noexisties") } @@ -480,14 +510,17 @@ class TestFileServer < Test::Unit::TestCase path #{basedir}/those " - } + } - # create a server with the file - assert_nothing_raised { - server = Puppet::Network::Handler.fileserver.new( - :Local => false, - :Config => conffile + # create a server with the file + assert_nothing_raised { + + server = Puppet::Network::Handler.fileserver.new( + + :Local => false, + + :Config => conffile ) } @@ -545,15 +578,16 @@ class TestFileServer < Test::Unit::TestCase case type when :deny - assert_raise(Puppet::AuthorizationError, - "Host %s, ip %s, allowed %s" % - [host, ip, mount]) { + + assert_raise( + Puppet::AuthorizationError, + + "Host %s, ip %s, allowed %s" % [host, ip, mount]) { list = server.list(mount, :manage, true, false, host, ip) } when :allow - assert_nothing_raised("Host %s, ip %s, denied %s" % - [host, ip, mount]) { - list = server.list(mount, :manage, true, false, host, ip) + assert_nothing_raised("Host %s, ip %s, denied %s" % [host, ip, mount]) { + list = server.list(mount, :manage, true, false, host, ip) } end } @@ -574,8 +608,8 @@ class TestFileServer < Test::Unit::TestCase " } - invalidconfigs = [ -"[not valid] + invalidconfigs = [ + "[not valid] path /this/path/does/not/exist allow 192.168.0.* ", @@ -589,24 +623,30 @@ class TestFileServer < Test::Unit::TestCase " ] - invalidmounts.each { |mount, text| - File.open(conffile, "w") { |f| - f.print text + invalidmounts.each { |mount, text| + File.open(conffile, "w") { |f| + f.print text } # create a server with the file server = nil assert_nothing_raised { + server = Puppet::Network::Handler.fileserver.new( + :Local => true, + :Config => conffile ) } - assert_raise(Puppet::Network::Handler::FileServerError, - "Invalid mount was mounted") { - server.list(mount, :manage) + + assert_raise( + Puppet::Network::Handler::FileServerError, + + "Invalid mount was mounted") { + server.list(mount, :manage) } } @@ -618,11 +658,17 @@ class TestFileServer < Test::Unit::TestCase # create a server with the file server = nil - assert_raise(Puppet::Network::Handler::FileServerError, - "Invalid config %s did not raise error" % i) { - server = Puppet::Network::Handler.fileserver.new( - :Local => true, - :Config => conffile + + assert_raise( + Puppet::Network::Handler::FileServerError, + + "Invalid config %s did not raise error" % i) { + + server = Puppet::Network::Handler.fileserver.new( + + :Local => true, + + :Config => conffile ) } } @@ -643,29 +689,34 @@ class TestFileServer < Test::Unit::TestCase path #{dir} allow test1.domain.com " - } + } - # Reset the timeout, so we reload faster - Puppet[:filetimeout] = 0.5 + # Reset the timeout, so we reload faster + Puppet[:filetimeout] = 0.5 - # start our server with a fast timeout - assert_nothing_raised { - server = Puppet::Network::Handler.fileserver.new( - :Local => false, - :Config => conffile + # start our server with a fast timeout + assert_nothing_raised { + + server = Puppet::Network::Handler.fileserver.new( + + :Local => false, + + :Config => conffile ) } list = nil assert_nothing_raised { - list = server.list("/thing/", :manage, false, false, + + list = server.list( + "/thing/", :manage, false, false, + "test1.domain.com", "127.0.0.1") } assert(list != "", "List returned nothing in rereard test") assert_raise(Puppet::AuthorizationError, "List allowed invalid host") { - list = server.list("/thing/", :manage, false, false, - "test2.domain.com", "127.0.0.1") + list = server.list("/thing/", :manage, false, false, "test2.domain.com", "127.0.0.1") } sleep 1 @@ -676,16 +727,14 @@ class TestFileServer < Test::Unit::TestCase path #{dir} allow test2.domain.com " - } + } - assert_raise(Puppet::AuthorizationError, "List allowed invalid host") { - list = server.list("/thing/", :manage, false, false, - "test1.domain.com", "127.0.0.1") + assert_raise(Puppet::AuthorizationError, "List allowed invalid host") { + list = server.list("/thing/", :manage, false, false, "test1.domain.com", "127.0.0.1") } assert_nothing_raised { - list = server.list("/thing/", :manage, false, false, - "test2.domain.com", "127.0.0.1") + list = server.list("/thing/", :manage, false, false, "test2.domain.com", "127.0.0.1") } assert(list != "", "List returned nothing in rereard test") @@ -716,8 +765,11 @@ class TestFileServer < Test::Unit::TestCase File.open(file, "w") { |f| f.puts "yay" } File.symlink(file, link) assert_nothing_raised { + server = Puppet::Network::Handler.fileserver.new( + :Local => true, + :Config => false ) } @@ -773,8 +825,7 @@ class TestFileServer < Test::Unit::TestCase client1_hostdir => "client1\n", client2_fqdndir => client2 + "\n" } - [fsdir, hostdir, fqdndir, - client1_hostdir, client2_fqdndir].each { |d| Dir.mkdir(d) } + [fsdir, hostdir, fqdndir, client1_hostdir, client2_fqdndir].each { |d| Dir.mkdir(d) } [client1_hostdir, client2_fqdndir].each do |d| File.open(File.join(d, "file.txt"), "w") do |f| @@ -791,13 +842,16 @@ allow * path #{fqdndir}/%H allow * ") - end + end - server = nil - assert_nothing_raised { - server = Puppet::Network::Handler.fileserver.new( - :Local => true, - :Config => conffile + server = nil + assert_nothing_raised { + + server = Puppet::Network::Handler.fileserver.new( + + :Local => true, + + :Config => conffile ) } @@ -938,34 +992,37 @@ allow * # Do a round of checks with a fake client client = "host.domain.com" {"%h" => "host", # Short name - "%H" => client, # Full name - "%d" => "domain.com", # domain - "%%" => "%", # escape - "%o" => "%o" # other - }.each do |pat, repl| - result = check.call(client, pat, repl) - end + "%H" => client, # Full name + "%d" => "domain.com", # domain + "%%" => "%", # escape + "%o" => "%o" # other + }.each do |pat, repl| + result = check.call(client, pat, repl) + end # Now, check that they use Facter info client = nil Facter.stubs(:value).with { |v| v.to_s == "hostname" }.returns("myhost") Facter.stubs(:value).with { |v| v.to_s == "domain" }.returns("mydomain.com") - Facter.stubs(:to_hash).returns({ - :ipaddress => "127.0.0.1", - :hostname => "myhost", - :domain => "mydomain.com", + + Facter.stubs(:to_hash).returns( + { + :ipaddress => "127.0.0.1", + :hostname => "myhost", + :domain => "mydomain.com", + }) {"%h" => "myhost", # Short name - "%H" => "myhost.mydomain.com", # Full name - "%d" => "mydomain.com", # domain - "%%" => "%", # escape - "%o" => "%o" # other - }.each do |pat, repl| - check.call(client, pat, repl) - end + "%H" => "myhost.mydomain.com", # Full name + "%d" => "mydomain.com", # domain + "%%" => "%", # escape + "%o" => "%o" # other + }.each do |pat, repl| + check.call(client, pat, repl) + end end @@ -973,8 +1030,11 @@ allow * def test_fileserver_expansion server = nil assert_nothing_raised { + server = Puppet::Network::Handler.fileserver.new( + :Local => true, + :Config => false ) } @@ -986,11 +1046,14 @@ allow * ip = '127.0.0.1' - Facter.stubs(:to_hash).returns({ - :kernel => kernel_fact, - :ipaddress => "127.0.0.1", - :hostname => "myhost", - :domain => "mydomain.com", + + Facter.stubs(:to_hash).returns( + { + :kernel => kernel_fact, + :ipaddress => "127.0.0.1", + :hostname => "myhost", + :domain => "mydomain.com", + }) Dir.mkdir(dir) @@ -1047,7 +1110,7 @@ allow * File::open(file, "w") { |f| f.print name } end - Puppet::Module::find(name) + Puppet::Module::find(name) end conffile = tempfile @@ -1057,8 +1120,11 @@ allow * # create a server with the file server = nil assert_nothing_raised { + server = Puppet::Network::Handler::FileServer.new( + :Local => false , + :Config => conffile ) } @@ -1080,8 +1146,7 @@ allow * end assert_nothing_raised("Host 'allow' denied #{mount}") { - server.list(mount, :manage, true, false, - 'allow.example.com', "192.168.0.1") + server.list(mount, :manage, true, false, 'allow.example.com', "192.168.0.1") } end end @@ -1109,14 +1174,17 @@ allow * path #{basedir}/thing allow 192.168.0.* " - } + } - # create a server with the file - server = nil - assert_nothing_raised { - server = Puppet::Network::Handler::FileServer.new( - :Local => false, - :Config => conffile + # create a server with the file + server = nil + assert_nothing_raised { + + server = Puppet::Network::Handler::FileServer.new( + + :Local => false, + + :Config => conffile ) } @@ -1136,14 +1204,15 @@ allow * assert_describe(mount, file, server) # now let's check that things are being correctly forbidden - assert_raise(Puppet::AuthorizationError, - "Host 'deny' allowed #{mount}") { - server.list(mount, :manage, true, false, - 'deny.example.com', "192.168.1.1") + + assert_raise( + Puppet::AuthorizationError, + + "Host 'deny' allowed #{mount}") { + server.list(mount, :manage, true, false, 'deny.example.com', "192.168.1.1") } assert_nothing_raised("Host 'allow' denied #{mount}") { - server.list(mount, :manage, true, false, - 'allow.example.com', "192.168.0.1") + server.list(mount, :manage, true, false, 'allow.example.com', "192.168.0.1") } end @@ -1161,8 +1230,11 @@ allow * ].each do |failer| File.open(config, "w") { |f| f.puts failer } assert_raise(Puppet::Network::Handler::FileServerError, "Did not fail on %s" % failer.inspect) { + server = Puppet::Network::Handler::FileServer.new( + :Local => false, + :Config => config ) } @@ -1172,8 +1244,8 @@ allow * def test_can_start_without_configuration Puppet[:fileserverconfig] = tempfile assert_nothing_raised("Could not create fileserver when configuration is absent") do - server = Puppet::Network::Handler::FileServer.new( - :Local => false + server = Puppet::Network::Handler::FileServer.new( + :Local => false ) end end -- cgit