Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

Move the directory maker into RSpec::Support #74

Merged
merged 1 commit into from
Jun 7, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions lib/rspec/support/directory_maker.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
module RSpec::Support
# @api private
#
# Replacement for fileutils#mkdir_p because we don't want to require parts
# of stdlib in RSpec.
class DirectoryMaker
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed this was a class. How about making it a module and the method a module_function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason why that's better? IMO leaving this as a class method is probably the simplest and most obvious thing to do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FileUtils is a module. It allows someone to mix it in if they don't want to call the top-level all the time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that usecase exists because FileUtils is a collection of library methods. On the other hand this object just encapsulates a single method and is RSpec internal use only. I'm not convinced I think that feature is necessary for this functionality.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@myronmarston @xaviershay @JonRowe any thoughts here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meh, it's private so I don't particularly care either way.

# @api private
#
# Implements nested directory construction
def self.mkdir_p(path)
stack = path.start_with?(File::SEPARATOR) ? File::SEPARATOR : "."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still feels *nix-ish only. I don't have a windows system to check on to see if this works or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to check that, pretty certain File::SEPARATOR is cross platform but unsure what window's would do with ./path/to/file

path.split(File::SEPARATOR).each do |part|
stack = File.join(stack, part)

begin
unless directory_exists?(stack)
Dir.mkdir(stack)
end
rescue Errno::ENOTDIR
raise Errno::EEXIST.new($!.message)
end

end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 This looks much cleaner ( ゚ヮ゚) Any reason for the multi-line unless? Just personal taste?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cupakromer will prevent churn in future if anything else needs to happen in that conditional. Makes diffs easier to read IMO.


private

def self.directory_exists?(dirname)
File.exist?(dirname) && File.directory?(dirname)
end
end
end
1 change: 1 addition & 0 deletions lib/rspec/support/spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
RSpec::Support.require_rspec_support "spec/with_isolated_stderr"
RSpec::Support.require_rspec_support "spec/stderr_splitter"
RSpec::Support.require_rspec_support "spec/formatting_support"
RSpec::Support.require_rspec_support "spec/with_isolated_directory"

warning_preventer = $stderr = RSpec::Support::StdErrSplitter.new($stderr)

Expand Down
9 changes: 9 additions & 0 deletions lib/rspec/support/spec/with_isolated_directory.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
require 'tmpdir'

RSpec.shared_context "isolated directory", :isolated_directory => true do
around do |ex|
Dir.mktmpdir do |tmp_dir|
Dir.chdir(tmp_dir, &ex)
end
end
end
58 changes: 58 additions & 0 deletions spec/rspec/support/directory_maker_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
require "spec_helper"
require "fileutils"

RSpec::Support.require_rspec_support("directory_maker")

module RSpec::Support
describe DirectoryMaker do
shared_examples_for "an mkdir_p implementation" do
include_context "isolated directory"

let(:dirname) { File.join(%w[tmp a recursive structure]) }

def directory_exists?(dirname)
File.exist?(dirname) && File.directory?(dirname)
end

it "makes directories recursively" do
mkdir_p.call(dirname)
expect(directory_exists?(dirname)).to be true
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is completely a personal style suggestion. Since the other spec is using the block form and this spec makes the assumption the directory didn't exist to start with, we can change to:

expect{
  DirectoryMaker.mkdir_p(dirname)
}.to change{ File.exist?(dirname) }.to true

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I think that's better, do you mind if I don't change it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Like I said it's more personal style. 😸

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it this way FWIW

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to use Dir.exist? instead of File.exist?.


it "does not raise if the directory already exists" do
Dir.mkdir("tmp")
mkdir_p.call(dirname)
expect(directory_exists?(dirname)).to be true
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer this to be a positive expectations:

Dir.mkdir("tmp")
DirectoryMaker.mkdir_p(dirname)
expect(Dir.exist?(dirname)).to be true

This verifies we actually create the remaining directories. Additionally, if an error is raised the spec will still fail.


context "when a file already exists" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Files always exist. Should this be "when the first part(s) of the path resolve to an existing file"?

before { File.open("tmp", "w") }

it "raises, as it can't make the directory" do
expect {
mkdir_p.call(dirname)
}.to raise_error(Errno::EEXIST)
end
end

context "when the path specified is absolute" do
let(:dirname) { "bees/ponies" }

it "makes directories recursively" do
mkdir_p.call(File.expand_path(dirname))
expect(directory_exists?(dirname)).to be true
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a Dir.exist?

end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the isolated directory helper changes, I think all specs are now based on relative paths. :-/ per let(:test_root) { "./some_directory" } being relative. I think you can drop test_root entirely at this point.

end

describe ".mkdir_p" do
subject(:mkdir_p) { DirectoryMaker.method(:mkdir_p) }
it_behaves_like "an mkdir_p implementation"
end

describe "FileUtils.mkdir_p" do
subject(:mkdir_p) { FileUtils.method(:mkdir_p) }
it_behaves_like "an mkdir_p implementation"
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest add the spec for the cases:

  • part of path is really a file (which is why we're using File.exist? instead of Dir.exist?)
  • providing a partial path instead of a full path (the let(:dirname) will always provide a full path due to how test_root is currently written); be to be explicit on full path and partial path behavior

end
end