diff options
| author | Markus Roberts <Markus@reality.com> | 2010-07-09 18:05:04 -0700 |
|---|---|---|
| committer | Markus Roberts <Markus@reality.com> | 2010-07-09 18:05:04 -0700 |
| commit | 9ee56f2e67be973da49b1d3f21de1bf87de35e6f (patch) | |
| tree | ddab8c01509f47664c52c8a6b165bb5a974f138f /spec/integration | |
| parent | 051bd98751d9d4bc97f93f66723d9b7a00c0cfb4 (diff) | |
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|
Diffstat (limited to 'spec/integration')
| -rwxr-xr-x | spec/integration/defaults_spec.rb | 10 | ||||
| -rw-r--r-- | spec/integration/indirector/report/rest_spec.rb | 5 | ||||
| -rwxr-xr-x | spec/integration/transaction_spec.rb | 55 | ||||
| -rwxr-xr-x | spec/integration/type/file_spec.rb | 25 |
4 files changed, 73 insertions, 22 deletions
diff --git a/spec/integration/defaults_spec.rb b/spec/integration/defaults_spec.rb index 3f66a0e8b..3fa753823 100755 --- a/spec/integration/defaults_spec.rb +++ b/spec/integration/defaults_spec.rb @@ -195,13 +195,13 @@ describe "Puppet defaults" do it "should have a setting for determining the configuration version and should default to an empty string" do Puppet.settings[:config_version].should == "" end - + describe "when enabling reports" do it "should use the default server value when report server is unspecified" do Puppet.settings[:server] = "server" Puppet.settings[:report_server].should == "server" end - + it "should use the default masterport value when report port is unspecified" do Puppet.settings[:masterport] = "1234" Puppet.settings[:report_port].should == "1234" @@ -211,20 +211,20 @@ describe "Puppet defaults" do Puppet.settings[:reportserver] = "reportserver" Puppet.settings[:report_server].should == "reportserver" end - + it "should use report_port when set" do Puppet.settings[:masterport] = "1234" Puppet.settings[:report_port] = "5678" Puppet.settings[:report_port].should == "5678" end - + it "should prefer report_server over reportserver" do Puppet.settings[:reportserver] = "reportserver" Puppet.settings[:report_server] = "report_server" Puppet.settings[:report_server].should == "report_server" end end - + it "should have a :caname setting that defaults to the cert name" do Puppet.settings[:certname] = "foo" Puppet.settings[:ca_name].should == "foo" diff --git a/spec/integration/indirector/report/rest_spec.rb b/spec/integration/indirector/report/rest_spec.rb index 76b3b9003..1ddced9f9 100644 --- a/spec/integration/indirector/report/rest_spec.rb +++ b/spec/integration/indirector/report/rest_spec.rb @@ -84,7 +84,10 @@ describe "Report REST Terminus" do } report.add_metric(:times, timemetrics) - report.add_metric(:changes, + + report.add_metric( + :changes, + :total => 20 ) diff --git a/spec/integration/transaction_spec.rb b/spec/integration/transaction_spec.rb index c93de1761..57dd490dd 100755 --- a/spec/integration/transaction_spec.rb +++ b/spec/integration/transaction_spec.rb @@ -106,20 +106,29 @@ describe Puppet::Transaction do path = tmpfile("path") file1 = tmpfile("file1") file2 = tmpfile("file2") - file = Puppet::Type.type(:file).new( + + file = Puppet::Type.type(:file).new( + :path => path, + :ensure => "file" ) - exec1 = Puppet::Type.type(:exec).new( + + exec1 = Puppet::Type.type(:exec).new( + :path => ENV["PATH"], :command => "touch %s" % file1, :refreshonly => true, + :subscribe => Puppet::Resource.new(:file, path) ) - exec2 = Puppet::Type.type(:exec).new( + + exec2 = Puppet::Type.type(:exec).new( + :path => ENV["PATH"], :command => "touch %s" % file2, :refreshonly => true, + :subscribe => Puppet::Resource.new(:file, path) ) @@ -132,24 +141,33 @@ describe Puppet::Transaction do it "should not let one failed refresh result in other refreshes failing" do path = tmpfile("path") newfile = tmpfile("file") - file = Puppet::Type.type(:file).new( + + file = Puppet::Type.type(:file).new( + :path => path, + :ensure => "file" ) - exec1 = Puppet::Type.type(:exec).new( + + exec1 = Puppet::Type.type(:exec).new( + :path => ENV["PATH"], :command => "touch /this/cannot/possibly/exist", :logoutput => true, :refreshonly => true, :subscribe => file, + :title => "one" ) - exec2 = Puppet::Type.type(:exec).new( + + exec2 = Puppet::Type.type(:exec).new( + :path => ENV["PATH"], :command => "touch %s" % newfile, :logoutput => true, :refreshonly => true, :subscribe => [file, exec1], + :title => "two" ) @@ -165,17 +183,23 @@ describe Puppet::Transaction do catalog.add_resource(*Puppet::Type.type(:schedule).mkdefaultschedules) Puppet[:ignoreschedules] = false - file = Puppet::Type.type(:file).new( + + file = Puppet::Type.type(:file).new( + :name => tmpfile("file"), + :ensure => "file", :backup => false ) fname = tmpfile("exec") - exec = Puppet::Type.type(:exec).new( + + exec = Puppet::Type.type(:exec).new( + :name => "touch #{fname}", :path => "/usr/bin:/bin", :schedule => "monthly", + :subscribe => Puppet::Resource.new("file", file.name) ) @@ -211,21 +235,30 @@ describe Puppet::Transaction do end it "should not attempt to evaluate resources with failed dependencies" do - exec = Puppet::Type.type(:exec).new( + + exec = Puppet::Type.type(:exec).new( + :command => "/bin/mkdir /this/path/cannot/possibly/exit", + :title => "mkdir" ) - file1 = Puppet::Type.type(:file).new( + + file1 = Puppet::Type.type(:file).new( + :title => "file1", :path => tmpfile("file1"), + :require => exec, :ensure => :file ) - file2 = Puppet::Type.type(:file).new( + + file2 = Puppet::Type.type(:file).new( + :title => "file2", :path => tmpfile("file2"), + :require => file1, :ensure => :file ) diff --git a/spec/integration/type/file_spec.rb b/spec/integration/type/file_spec.rb index f215f0b08..e16da67cc 100755 --- a/spec/integration/type/file_spec.rb +++ b/spec/integration/type/file_spec.rb @@ -392,8 +392,11 @@ describe Puppet::Type.type(:file) do it "should be able to create files when 'content' is specified but 'ensure' is not" do dest = tmpfile("files_with_content") - file = Puppet::Type.type(:file).new( + + file = Puppet::Type.type(:file).new( + :name => dest, + :content => "this is some content, yo" ) @@ -407,9 +410,12 @@ describe Puppet::Type.type(:file) do it "should create files with content if both 'content' and 'ensure' are set" do dest = tmpfile("files_with_content") - file = Puppet::Type.type(:file).new( + + file = Puppet::Type.type(:file).new( + :name => dest, :ensure => "file", + :content => "this is some content, yo" ) @@ -426,10 +432,13 @@ describe Puppet::Type.type(:file) do File.open(source, "w") { |f| f.puts "yay" } File.open(dest, "w") { |f| f.puts "boo" } - file = Puppet::Type.type(:file).new( + + file = Puppet::Type.type(:file).new( + :name => dest, :ensure => :absent, :source => source, + :backup => false ) @@ -455,18 +464,24 @@ describe Puppet::Type.type(:file) do # this file should get removed File.open(@purgee, "w") { |f| f.puts "footest" } - @lfobj = Puppet::Type.newfile( + + @lfobj = Puppet::Type.newfile( + :title => "localfile", :path => @localfile, :content => "rahtest\n", :ensure => :file, + :backup => false ) - @destobj = Puppet::Type.newfile(:title => "destdir", :path => @destdir, + + @destobj = Puppet::Type.newfile( + :title => "destdir", :path => @destdir, :source => @sourcedir, :backup => false, :purge => true, + :recurse => true) @catalog = Puppet::Resource::Catalog.new |
