On Tue, Jul 19, 2011 at 14:59, Josh Cooper <j...@puppetlabs.com> wrote:
> Absolute paths on Unix, e.g. /foo/bar, are not absolute on Windows,
> which breaks many test cases. This commit adds a method to
> PuppetSpec::Files.make_absolute that makes the path absolute in
> test cases.
>
> On Unix (Puppet.features.posix?) it is a no-op. On Windows,
> (Puppet.features.microsoft_windows?) the drive from the current
> working directory is prepended.
>
> Reviewed-by: Jacob Helwig <ja...@puppetlabs.com>
> Signed-off-by: Josh Cooper <j...@puppetlabs.com>
> ---
Generally, it looks like you used member variables rather than let
methods for constants in these tests. It would be nice to migrate
toward the more modern and less leaky tools that rspec gives us these
days.
> --- a/spec/integration/transaction_spec.rb
> +++ b/spec/integration/transaction_spec.rb
> @@ -1,9 +1,7 @@
> #!/usr/bin/env rspec
> require 'spec_helper'
>
> -require 'puppet_spec/files'
> require 'puppet/transaction'
> -require 'puppet_spec/files'
That change looks unrelated to the stated purpose of the changeset. I
assume that these are redundant requires to something more global, but
it would have been good to call that out.
> it "should not fail if the path is fully qualified, with a trailing
> separator" do
> - path = "/some/path/with/trailing/separator"
> - path_with_separator = "#{path}#{File::SEPARATOR}"
> - File.stubs(:lstat).with(path).returns stub('stat')
> + path_with_separator = "#{@somefile}#{File::SEPARATOR}"
> + File.stubs(:lstat).with(@somefile).returns stub('stat')
Those are not exhaustive tests for the platform; this might leak
through File::ALT_SEPARATOR. The tests should cover all the options
and combinations there.
> it "should accept a 'links' option" do
> - File.expects(:lstat).with("/some/file").returns stub("stat")
> - set = Puppet::FileServing::Fileset.new("/some/file", :links => :manage)
> + File.expects(:lstat).with(@somefile).returns stub("stat")
> + set = Puppet::FileServing::Fileset.new(@somefile, :links => :manage)
> set.links.should == :manage
> end
This should be blowing up on Win32, where links don't work, I would
imagine. Is this test stubbing too much, and so testing only that the
assumptions we coded in our stubs hold true?
> # this is the default from spec_helper, but it keeps getting reset at
> odd times
> - Puppet[:bucketdir] = "/dev/null/bucket"
> + Puppet[:bucketdir] = make_absolute("/dev/null/bucket")
So, I understand that part of the goal with these names was that they
would cause things to blow up or fail if they were looked through. I
have not actually *verified* that, but these would be all legal on
Win32 and all, which means the tests either don't depend on that, or
that we might not be testing what we imagine...
> @@ -38,7 +40,7 @@ require 'spec_helper'
> it "should retrieve #{param} if a SELinux context is found without a
> range" do
> stat = stub 'stat', :ftype => "foo"
> @resource.expects(:stat).returns stat
> - @sel.expects(:get_selinux_current_context).with("/my/file").returns
> "user_u:role_r:type_t"
> + @sel.expects(:get_selinux_current_context).with(@path).returns
> "user_u:role_r:type_t"
Wouldn't it be better to just turn off tests that are irrelevant to,
and can never pass on, the platform?
Daniel
--
⎋ Puppet Labs Developer – http://puppetlabs.com
✉ Daniel Pittman <dan...@puppetlabs.com>
✆ Contact me via gtalk, email, or phone: +1 (877) 575-9775
♲ Made with 100 percent post-consumer electrons
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com.
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
