New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove pathname require from ruby_project.rb #1703
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,33 @@ module Core | |
end | ||
|
||
end | ||
|
||
describe "#ascend_until" do | ||
subject { RubyProject } | ||
|
||
def expect_ascend(source_path, *yielded_paths) | ||
expect { |probe| | ||
allow(File).to receive(:expand_path).with('.') { source_path } | ||
subject.ascend_until(&probe) | ||
}.to yield_successive_args(*yielded_paths) | ||
end | ||
|
||
it "works with a normal path" do | ||
expect_ascend("/var//ponies/", "/var/ponies", "/var", "/") | ||
end | ||
|
||
it "works with a path with a trailing slash" do | ||
expect_ascend("/var//ponies/", "/var/ponies", "/var", "/") | ||
end | ||
|
||
it "works with a path with double slashes" do | ||
expect_ascend("/var//ponies/", "/var/ponies", "/var", "/") | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The three paths in test are all the same. According to the descriptions, the paths should be |
||
|
||
it "works with a path with escaped slashes" do | ||
expect_ascend("/var\\/ponies/", "/var\\/ponies", "/") | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These specs all follow the same form. I think it's worth extracting a common helper method: def expect_ascend(source_path, *yielded_paths)
allow(File).to receive(:expand_path).with('.') { source_path }
expect { |probe|
subject.ascend_until(&probe)
}.to yield_successive_args(*yielded_paths)
end
# call like:
expect_ascend("/var//ponies/", "/var/ponies", "/var", "/") Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✨ |
||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this before block needed? You explicitly stub
expand_path
in each of the specs below...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that JRuby calls expand_path as part of it's require mechanism, so this is required to allow JRuby to require files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to ask "what is requiring a file here?"...but then I tried it locally and I see it now: it's our autoload mechanism for matchers in rspec-expectations, when
yield_successive_args
is called. IMO, it would be better to changeexpect_ascend
to this:With that change in place, you don't need this odd stub.