From 5d56da6bddcaa4d510fd97a8e38d80e750e3e20f Mon Sep 17 00:00:00 2001 From: Izaak Alpert Date: Thu, 6 Jun 2013 17:22:36 -0400 Subject: [PATCH] MR on fork: Some cleanup, test updates -The forked merge request test now tests it's componenets again, and seems to work every time (did this by reordering the branch updates so their is more time for update_branches to run) -- this could still technically fail, but after several runs it doesn't seem to. -Removed todo in merge_request, pushed wrapping of grit down to the satellite -updated action test to check flock status, made it nolonger pending -moved all logging on failure to helper method in satellite GITLAB-592 Change-Id: If0554ca35eedc3d3e8461f7d93d4b3939fa2cd75 --- app/assets/stylesheets/common.scss | 1 + app/models/merge_request.rb | 29 ++++----- .../project/project_forked_merge_requests.rb | 53 +++++++++------- lib/gitlab/satellite/action.rb | 13 ++-- lib/gitlab/satellite/merge_action.rb | 24 +++----- spec/lib/gitlab/satellite/action_spec.rb | 61 ++++++++----------- .../lib/gitlab/satellite/merge_action_spec.rb | 30 +++++---- 7 files changed, 103 insertions(+), 108 deletions(-) diff --git a/app/assets/stylesheets/common.scss b/app/assets/stylesheets/common.scss index 2f98df3fe41..f1b7853fa69 100644 --- a/app/assets/stylesheets/common.scss +++ b/app/assets/stylesheets/common.scss @@ -415,6 +415,7 @@ img.emoji { @extend .light-well; @extend .light; margin-bottom: 10px; +} .label-project { @include border-radius(4px); diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 0e0ae3c3a07..050dc4910f2 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -26,12 +26,10 @@ class MergeRequest < ActiveRecord::Base include Issuable - belongs_to :target_project,:foreign_key => :target_project_id, class_name: "Project" - belongs_to :source_project, :foreign_key => :source_project_id,class_name: "Project" + belongs_to :target_project, :foreign_key => :target_project_id, class_name: "Project" + belongs_to :source_project, :foreign_key => :source_project_id, class_name: "Project" - BROKEN_DIFF = "--broken-diff" - - attr_accessible :title, :assignee_id, :source_project_id, :source_branch, :target_project_id, :target_branch, :milestone_id,:author_id_of_changes, :state_event + attr_accessible :title, :assignee_id, :source_project_id, :source_branch, :target_project_id, :target_branch, :milestone_id, :author_id_of_changes, :state_event attr_accessor :should_remove_source_branch @@ -87,8 +85,8 @@ class MergeRequest < ActiveRecord::Base validates :target_branch, presence: true validate :validate_branches - scope :of_group, ->(group) { where("source_project_id in (:group_project_ids) OR target_project_id in (:group_project_ids)",group_project_ids:group.project_ids) } - scope :of_user_team, ->(team) { where("(source_project_id in (:team_project_ids) OR target_project_id in (:team_project_ids) AND assignee_id in (:team_member_ids))",team_project_ids:team.project_ids,team_member_ids:team.member_ids) } + scope :of_group, ->(group) { where("source_project_id in (:group_project_ids) OR target_project_id in (:group_project_ids)", group_project_ids: group.project_ids) } + scope :of_user_team, ->(team) { where("(source_project_id in (:team_project_ids) OR target_project_id in (:team_project_ids) AND assignee_id in (:team_member_ids))", team_project_ids: team.project_ids, team_member_ids: team.member_ids) } scope :opened, -> { with_state(:opened) } scope :closed, -> { with_state(:closed) } scope :merged, -> { with_state(:merged) } @@ -151,13 +149,8 @@ class MergeRequest < ActiveRecord::Base end def unmerged_diffs - #TODO:[IA-8] this needs to be handled better -- logged etc - diffs = Gitlab::Satellite::MergeAction.new(author, self).diffs_between_satellite - if diffs - diffs = diffs.map { |diff| Gitlab::Git::Diff.new(diff) } - else - diffs = [] - end + diffs = Gitlab::Satellite::MergeAction.new(author, self).diffs_between_satellite + diffs ||= [] diffs end @@ -192,12 +185,11 @@ class MergeRequest < ActiveRecord::Base end def unmerged_commits - commits = Gitlab::Satellite::MergeAction.new(self.author,self).commits_between - commits = commits.map{ |commit| Gitlab::Git::Commit.new(commit, nil) } + commits = Gitlab::Satellite::MergeAction.new(self.author, self).commits_between if commits.present? commits = Commit.decorate(commits). - sort_by(&:created_at). - reverse + sort_by(&:created_at). + reverse end commits end @@ -222,6 +214,7 @@ class MergeRequest < ActiveRecord::Base commit_ids = commits.map(&:id) Note.where("(noteable_type = 'MergeRequest' AND noteable_id = :mr_id) OR (noteable_type = 'Commit' AND commit_id IN (:commit_ids))", mr_id: id, commit_ids: commit_ids) end + # Returns the raw diff for this merge request # # see "git diff" diff --git a/features/steps/project/project_forked_merge_requests.rb b/features/steps/project/project_forked_merge_requests.rb index cbd59a4b7a1..10d26a1498b 100644 --- a/features/steps/project/project_forked_merge_requests.rb +++ b/features/steps/project/project_forked_merge_requests.rb @@ -5,6 +5,7 @@ class ProjectForkedMergeRequests < Spinach::FeatureSteps include SharedPaths + Given 'I am a member of project "Shop"' do @project = Project.find_by_name "Shop" @project ||= create(:project_with_code, name: "Shop") @@ -34,22 +35,42 @@ class ProjectForkedMergeRequests < Spinach::FeatureSteps current_path.should == project_merge_request_path(@project, @merge_request) @merge_request.title.should == "Merge Request On Forked Project" @merge_request.source_project.should == @forked_project + @merge_request.source_branch.should == "master" + @merge_request.target_branch.should == "stable" + page.should have_content @forked_project.path_with_namespace + page.should have_content @project.path_with_namespace + page.should have_content @merge_request.source_branch + page.should have_content @merge_request.target_branch end And 'I fill out a "Merge Request On Forked Project" merge request' do + #The ordering here is a bit whacky on purpose: + #Select the target right away, to give update_branches time to run and clean up the target_branches + find(:select, "merge_request_target_project_id", {}).value.should == @forked_project.id.to_s + select @project.path_with_namespace, from: "merge_request_target_project_id" + + fill_in "merge_request_title", with: "Merge Request On Forked Project" find(:select, "merge_request_source_project_id", {}).value.should == @forked_project.id.to_s - find(:select, "merge_request_target_project_id", {}).value.should == @forked_project.id.to_s - select @project.path_with_namespace, from: "merge_request_target_project_id" find(:select, "merge_request_target_project_id", {}).value.should == @project.id.to_s - select "master", from: "merge_request_source_branch" - find(:select, "merge_request_source_branch", {}).value.should have_content "master" - #Force the page to wait until the javascript finishes, so the stable option shows up - select "stable", from: "merge_request_target_branch" - find(:select, "merge_request_target_branch", {}).should have_content "stable" + #Ensure the option exists in the select + find(:select, "merge_request_source_branch", {}).should have_content "master" + select "master", from: "merge_request_source_branch" + #Ensure the option is selected + find(:select, "merge_request_source_branch", {}).value.should have_content "master" verify_commit_link(".mr_source_commit",@forked_project) + + + #This could fail if the javascript hasn't run yet, there is a timing issue here -- this is why we do the select at the top + #Ensure the option exists in the select + find(:select, "merge_request_target_branch", {}).should have_content "stable" + #We must give apparently lots of time for update branches to finish + + (find(:select, "merge_request_target_branch", {}).find(:option, "stable",{}).select_option).should be_true + #Ensure the option is selected + find(:select, "merge_request_target_branch", {}).value.should have_content "stable" verify_commit_link(".mr_target_commit",@project) end @@ -126,14 +147,8 @@ class ProjectForkedMergeRequests < Spinach::FeatureSteps find("#merge_request_target_project_id").value.should == @project.id.to_s find("#merge_request_source_branch").value.should have_content "master" verify_commit_link(".mr_source_commit",@forked_project) - #TODO[IA-08] reienstate these -- not sure why they started repeatedly breaking - #sleep 3 - #puts "Source text:#{find("#merge_request_source_branch").text}" - #puts "Source value:#{find("#merge_request_source_branch").value}" - #puts "Target text:#{find("#merge_request_target_branch").text}" - #puts "Target value:#{find("#merge_request_target_branch").value}" - #find("#merge_request_target_branch").value.should have_content "stable" - #verify_commit_link(".mr_target_commit",@project) + find("#merge_request_target_branch").value.should have_content "stable" + verify_commit_link(".mr_target_commit",@project) end @@ -144,16 +159,8 @@ class ProjectForkedMergeRequests < Spinach::FeatureSteps #Verify a link is generated against the correct project def verify_commit_link(container_div, container_project) #This should force a wait for the javascript to execute - #puts "text: #{find(:div,container_div).text}" find(:div,container_div).should have_css ".browse_code_link_holder" find(:div,container_div).find(".commit_short_id")['href'].should have_content "#{container_project.path_with_namespace}/commit" - #commit_links = commit.all("a").map(&:text) - #links = commit_links.collect {|e|commit.find(:link,e)['href']} - #links.size.should == 4 - #links[0].should have_content "#{container_project.path_with_namespace}/tree" - #links[1].should have_content "#{container_project.path_with_namespace}/commit" - #links[3].should have_content "#{container_project.path_with_namespace}/commit" - end end diff --git a/lib/gitlab/satellite/action.rb b/lib/gitlab/satellite/action.rb index f22d595d5cd..bb93797c1f4 100644 --- a/lib/gitlab/satellite/action.rb +++ b/lib/gitlab/satellite/action.rb @@ -1,7 +1,7 @@ module Gitlab module Satellite class Action - DEFAULT_OPTIONS = { git_timeout: 30.seconds} + DEFAULT_OPTIONS = {git_timeout: 30.seconds} attr_accessor :options, :project, :user @@ -25,11 +25,9 @@ module Gitlab end end rescue Errno::ENOMEM => ex - Gitlab::GitLogger.error(ex.message) - return false + return handle_exception(ex) rescue Grit::Git::GitTimeout => ex - Gitlab::GitLogger.error(ex.message) - return false + return handle_exception(ex) ensure Gitlab::ShellEnv.reset_env end @@ -48,6 +46,11 @@ module Gitlab def default_options(options = {}) {raise: true, timeout: true}.merge(options) end + + def handle_exception(exception) + Gitlab::GitLogger.error(exception.message) + false + end end end end diff --git a/lib/gitlab/satellite/merge_action.rb b/lib/gitlab/satellite/merge_action.rb index 22c10515d2a..604b2ca9a19 100644 --- a/lib/gitlab/satellite/merge_action.rb +++ b/lib/gitlab/satellite/merge_action.rb @@ -41,8 +41,7 @@ module Gitlab end end rescue Grit::Git::CommandFailed => ex - Gitlab::GitLogger.error(ex.message) - false + handle_exception(ex) end @@ -61,8 +60,7 @@ module Gitlab return diff end rescue Grit::Git::CommandFailed => ex - Gitlab::GitLogger.error(ex.message) - false + handle_exception(ex) end # Only show what is new in the source branch compared to the target branch, not the other way around. @@ -81,11 +79,11 @@ module Gitlab #this method doesn't take default options diffs = merge_repo.diff(common_commit, "#{merge_request.source_branch}") end + diffs = diffs.map { |diff| Gitlab::Git::Diff.new(diff) } return diffs end rescue Grit::Git::CommandFailed => ex - Gitlab::GitLogger.error(ex.message) - false + handle_exception(ex) end # Get commit as an email patch @@ -101,8 +99,7 @@ module Gitlab return patch end rescue Grit::Git::CommandFailed => ex - Gitlab::GitLogger.error(ex.message) - false + handle_exception(ex) end # Retrieve an array of commits between the source and the target @@ -115,12 +112,12 @@ module Gitlab else commits = merge_repo.commits_between("#{merge_request.target_branch}", "#{merge_request.source_branch}") end + commits = commits.map { |commit| Gitlab::Git::Commit.new(commit, nil) } return commits end rescue Grit::Git::CommandFailed => ex - Gitlab::GitLogger.error(ex.message) - false + handle_exception(ex) end private @@ -142,8 +139,7 @@ module Gitlab repo.git.pull(default_options({no_ff: true}), 'origin', merge_request.source_branch) end rescue Grit::Git::CommandFailed => ex - Gitlab::GitLogger.error(ex.message) - false + handle_exception(ex) end # Assumes a satellite exists that is a fresh clone of the projects repo, prepares satellite for merges, diffs etc @@ -159,11 +155,9 @@ module Gitlab repo.git.checkout(default_options, "#{merge_request.target_branch}") end rescue Grit::Git::CommandFailed => ex - Gitlab::GitLogger.error(ex.message) - false + handle_exception(ex) end - end end end diff --git a/spec/lib/gitlab/satellite/action_spec.rb b/spec/lib/gitlab/satellite/action_spec.rb index 3a2dd0bf6d3..e37edb9614b 100644 --- a/spec/lib/gitlab/satellite/action_spec.rb +++ b/spec/lib/gitlab/satellite/action_spec.rb @@ -78,52 +78,43 @@ describe 'Gitlab::Satellite::Action' do end it 'should be able to use the satellite after locking' do - pending "can't test this, doesn't seem to be a way to the the flock status on a file, throwing piles of processes at it seems lousy too" repo = project.satellite.repo - first_call = false - - (File.exists? project.satellite.lock_file).should be_false - - test_file = ->(called) { - File.exists?(project.satellite.lock_file).should be_true - called.should be_true - File.readlines.should == "some test code" - File.truncate(project.satellite.lock, 0) - File.readlines.should == "" - } - - write_file = ->(called, checker) { - if (File.exists?(project.satellite.lock_file)) - file = File.open(project.satellite.lock, '+w') - file.write("some test code") - file.close - checker.call(called) - end - } + called = false + # Set base assumptions + if File.exists? project.satellite.lock_file + FileLockStatusChecker.new(project.satellite.lock_file).flocked?.should be_false + end satellite_action = Gitlab::Satellite::Action.new(user, project) satellite_action.send(:in_locked_and_timed_satellite) do |sat_repo| - write_file.call(first_call, test_file) - first_call = true + called = true repo.should == sat_repo (File.exists? project.satellite.lock_file).should be_true - + FileLockStatusChecker.new(project.satellite.lock_file).flocked?.should be_true end - first_call.should be_true - puts File.stat(project.satellite.lock_file).inspect + called.should be_true + FileLockStatusChecker.new(project.satellite.lock_file).flocked?.should be_false - second_call = false - satellite_action.send(:in_locked_and_timed_satellite) do |sat_repo| - write_file.call(second_call, test_file) - second_call = true - repo.should == sat_repo - (File.exists? project.satellite.lock_file).should be_true + end + + class FileLockStatusChecker < File + def flocked? &block + status = flock LOCK_EX|LOCK_NB + case status + when false + return true + when 0 + begin + block ? block.call : false + ensure + flock LOCK_UN + end + else + raise SystemCallError, status + end end - - second_call.should be_true - (File.exists? project.satellite.lock_file).should be_true end end diff --git a/spec/lib/gitlab/satellite/merge_action_spec.rb b/spec/lib/gitlab/satellite/merge_action_spec.rb index d0b59d379c8..3f6e5c35245 100644 --- a/spec/lib/gitlab/satellite/merge_action_spec.rb +++ b/spec/lib/gitlab/satellite/merge_action_spec.rb @@ -8,7 +8,7 @@ describe 'Gitlab::Satellite::MergeAction' do @wiki_branch = ['wiki', '635d3e09b72232b6e92a38de6cc184147e5bcb41'] #this is the commit sha where the wiki branch goes off from master @conflicting_metior = ['metior', '313d96e42b313a0af5ab50fa233bf43e27118b3f'] #this branch conflicts with the wiki branch - #these commits are quite close together, itended to make string diffs/format patches small + #these commits are quite close together, itended to make string diffs/format patches small @close_commit1 = ['2_3_notes_fix', '8470d70da67355c9c009e4401746b1d5410af2e3'] @close_commit2 = ['scss_refactoring', 'f0f14c8eaba69ebddd766498a9d0b0e79becd633'] @@ -18,19 +18,23 @@ describe 'Gitlab::Satellite::MergeAction' do let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } let(:merge_request_fork) { create(:merge_request) } describe '#commits_between' do + def verify_commits(commits, first_commit_sha, last_commit_sha) + commits.each { |commit| commit.class.should == Gitlab::Git::Commit } + commits.first.id.should == first_commit_sha + commits.last.id.should == last_commit_sha + end + context 'on fork' do it 'should get proper commits between' do merge_request_fork.target_branch = @one_after_stable[0] merge_request_fork.source_branch = @master[0] commits = Gitlab::Satellite::MergeAction.new(merge_request_fork.author, merge_request_fork).commits_between - commits.first.id.should == @one_after_stable[1] - commits.last.id.should == @master[1] + verify_commits(commits, @one_after_stable[1], @master[1]) merge_request_fork.target_branch = @wiki_branch[0] merge_request_fork.source_branch = @master[0] commits = Gitlab::Satellite::MergeAction.new(merge_request_fork.author, merge_request_fork).commits_between - commits.first.id.should == @wiki_branch[1] - commits.last.id.should == @master[1] + verify_commits(commits, @wiki_branch[1], @master[1]) end end @@ -39,14 +43,12 @@ describe 'Gitlab::Satellite::MergeAction' do merge_request.target_branch = @one_after_stable[0] merge_request.source_branch = @master[0] commits = Gitlab::Satellite::MergeAction.new(merge_request.author, merge_request).commits_between - commits.first.id.should == @one_after_stable[1] - commits.last.id.should == @master[1] + verify_commits(commits, @one_after_stable[1], @master[1]) merge_request.target_branch = @wiki_branch[0] merge_request.source_branch = @master[0] commits = Gitlab::Satellite::MergeAction.new(merge_request.author, merge_request).commits_between - commits.first.id.should == @wiki_branch[1] - commits.last.id.should == @master[1] + verify_commits(commits, @wiki_branch[1], @master[1]) end end end @@ -81,8 +83,12 @@ describe 'Gitlab::Satellite::MergeAction' do diff_count = diff.scan('diff --git').size diff_count.should >= 1 diffs.size.should == diff_count - diffs.each {|a_diff| (diff.include? a_diff.diff).should be_true} + diffs.each do |a_diff| + a_diff.class.should == Gitlab::Git::Diff + (diff.include? a_diff.diff).should be_true + end end + context 'on fork' do it 'should get proper diffs' do merge_request_fork.target_branch = @close_commit1[0] @@ -93,7 +99,7 @@ describe 'Gitlab::Satellite::MergeAction' do merge_request_fork.source_branch = @master[0] diff = Gitlab::Satellite::MergeAction.new(merge_request.author, merge_request_fork).diff_in_satellite - is_a_matching_diff(diff,diffs) + is_a_matching_diff(diff, diffs) end end @@ -108,7 +114,7 @@ describe 'Gitlab::Satellite::MergeAction' do merge_request.source_branch = @master[0] diff = Gitlab::Satellite::MergeAction.new(merge_request.author, merge_request).diff_in_satellite - is_a_matching_diff(diff,diffs) + is_a_matching_diff(diff, diffs) end end end