-
Notifications
You must be signed in to change notification settings - Fork 481
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
[BUGFIX] Fix Danger::GitRepo::find_merge_base_with_incremental_fetch
return value
#1498
[BUGFIX] Fix Danger::GitRepo::find_merge_base_with_incremental_fetch
return value
#1498
Conversation
fix to return merge_base not boolean
Danger::GitRepo::find_merge_base_with_incremental_fetch
return valueDanger::GitRepo::find_merge_base_with_incremental_fetch
return value
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.
@nagataaaas
Thank you for your contribution 🥇
The purpose of this PR seems reasonable to me, but there appears to be a minor bug. Could you please fix it?
📝 I wrote a couple of small and ad-hoc test cases to check this PR. the latter one diff --git a/spec/lib/danger/scm_source/git_repo_spec.rb b/spec/lib/danger/scm_source/git_repo_spec.rb
index 6509d27a..689b0461 100644
--- a/spec/lib/danger/scm_source/git_repo_spec.rb
+++ b/spec/lib/danger/scm_source/git_repo_spec.rb
@@ -269,4 +269,41 @@ RSpec.describe Danger::GitRepo, host: :github do
end
end
end
+
+ describe "#find_merge_base_with_incremental_fetch" do
+ it "returns SHA1 hash of the possible merge base if any" do
+ Dir.mktmpdir do |dir|
+ Dir.chdir(dir) do
+ `git clone --depth=1 --single-branch --branch=master git@github.com:danger/danger.git`
+ repo_path = File.join(dir, "danger")
+ Dir.chdir(repo_path) do
+ @dm = testing_dangerfile
+ repo = Git.open(repo_path)
+ # https://github.com/danger/danger/commit/16ebebd8045667dbd034c106d4a4deb5dbde1cba
+ from = "16ebebd8045667dbd034c106d4a4deb5dbde1cba"
+ to = "master"
+ possible_merge_base = @dm.env.scm.send(:find_merge_base_with_incremental_fetch, repo, from, to)
+ expect(possible_merge_base).to match /[0-9a-f]{40}/
+ end
+ end
+ end
+ end
+
+ it "returns nil when no possible merge base is found" do
+ Dir.mktmpdir do |dir|
+ Dir.chdir(dir) do
+ `git clone --depth=1 --single-branch --branch=master git@github.com:danger/danger.git`
+ repo_path = File.join(dir, "danger")
+ Dir.chdir(repo_path) do
+ @dm = testing_dangerfile
+ repo = Git.open(repo_path)
+ from = "0000000000000000000000000000000000000000"
+ to = "master"
+ possible_merge_base = @dm.env.scm.send(:find_merge_base_with_incremental_fetch, repo, from, to)
+ expect(possible_merge_base).to be_nil
+ end
+ end
+ 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.
LGTM, Thank you @nagataaaas 👍
Looks like there's a bug.
In the method
Danger::GitRepo::find_merge_base_with_incremental_fetch
which supposed to return string(possible_merge_base), we return boolean(possible_merge_base is found).This causes an error that passes string
true
orfalse
as a branch to git.