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

2 files changed, 44 insertions, 47 deletions

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
+    end
 
-      second_call.should be_true
-      (File.exists? project.satellite.lock_file).should be_true
+    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
     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