completed CR with @tucker250

models/notes_generator.rb → lib/build_notes.rb

@@ -1,5 +1,5 @@
-class NotesGenerator
-  include Builder
+class BuildNotes
+  include BuildParticipant
 
   def notes
     JSON.pretty_generate(generate_notes_hash)

models/builder.rb → lib/build_participant.rb

@@ -1,6 +1,6 @@
 require 'stringio'
 
-module Builder
+module BuildParticipant
   def build
     BuildContext.instance.build
   end
@@ -13,6 +13,26 @@ def listener
     BuildContext.instance.listener
   end
 
+  def debug(line)
+    listener.debug(format(line))
+  end
+
+  def error(line)
+    listener.error(format(line))
+  end
+
+  def fatal(line)
+    listener.fatal(format(line))
+  end
+
+  def info(line)
+    listener.info(format(line))
+  end
+
+  def warn(line)
+    listener.warn(format(line))
+  end
+
   def run(command, opts = {})
     info "running command #{command.inspect} with opts #{opts.inspect}"
 
@@ -39,26 +59,6 @@ def run(command, opts = {})
     result
   end
 
-  def debug(line)
-    listener.debug(format(line))
-  end
-
-  def error(line)
-    listener.error(format(line))
-  end
-
-  def fatal(line)
-    listener.fatal(format(line))
-  end
-
-  def info(line)
-    listener.info(format(line))
-  end
-
-  def warn(line)
-    listener.warn(format(line))
-  end
-
   def format(line)
     "#{Constants::LOG_PREFIX}#{line}"
   end

models/git_updater.rb → lib/git_updater.rb

@@ -1,18 +1,17 @@
 class GitUpdater
-  include Builder
+  class ConcurrentUpdateError < RuntimeError ; end
 
-  attr_reader :notes
+  include BuildParticipant
 
   def refname
     Constants::GIT_NOTES_REF
   end
 
   # Attach a git note based on the build status and push it to origin
-  def update!
+  def update!(notes)
     fetch_notes
     show_notes
-    generate_notes
-    push_notes
+    push_notes(notes)
   end
 
   # Force-update the notes ref to get changes from other builds
@@ -32,19 +31,10 @@ def show_notes
     end
   end
 
-  def generate_notes
-    info "generating notes"
-    @notes = NotesGenerator.new.notes
-    info "new notes: #{notes}"
-  end
-
   # Add the git note and push it to origin
-  def push_notes
+  def push_notes(notes)
     run("git notes --ref #{refname} add -f -F -", {:stdin_str => notes, :raise => true})
-    begin
-      run("git push origin refs/notes/#{refname}", {:raise => true})
-    rescue RuntimeError => e
-      raise(ConcurrentUpdateError, "trouble pushing notes: #{e.inspect}", e.backtrace)
-    end
+    ret = run("git push origin refs/notes/#{refname}")
+    raise(ConcurrentUpdateError, "trouble pushing notes") unless ret[:val] == 0
   end
 end

models/git_notes_publisher.rb

@@ -1,5 +1,11 @@
+require File.expand_path('../../lib/constants', __FILE__)
+require File.expand_path('../../lib/build_context', __FILE__)
+require File.expand_path('../../lib/build_participant', __FILE__)
+require File.expand_path('../../lib/build_notes', __FILE__)
+require File.expand_path('../../lib/git_updater', __FILE__)
+
 class GitNotesPublisher < Jenkins::Tasks::Publisher
-  include Builder
+  include BuildParticipant
 
   display_name "Publish build result as git-notes"
 
@@ -24,10 +30,11 @@ def perform(build, launcher, listener)
     BuildContext.instance.set(build, launcher, listener) do
       git_updater = GitUpdater.new
       retries = Constants::CONCURRENT_UPDATE_RETRIES
+      notes = BuildNotes.new.notes
       begin
         info "updating git notes"
-        git_updater.update!
-      rescue ConcurrentUpdateError => e
+        git_updater.update!(notes)
+      rescue GitUpdater::ConcurrentUpdateError => e
         if retries > 0
           warn "caught ConcurrentUpdateError while updating git notes, retrying (#{retries}x left)"
           retries -= 1
@@ -36,28 +43,7 @@ def perform(build, launcher, listener)
           raise e
         end
       end
+      info "updated git notes: #{notes}"
     end
   end
-
-  # Return a hash representing the git note we want to add to HEAD
-  private
-  def build_note_hash(build)
-    native = build.send(:native)
-    built_on = native.getBuiltOnStr || "master"
-    built_on = "master" if built_on.empty?
-    time = Time.at(native.getTimeInMillis / 1000.0)
-    duration = Time.now - time
-
-    {
-      :built_on => built_on,
-      :duration => duration,
-      :full_display_name => native.getFullDisplayName,
-      :id => native.getId,
-      :number => native.getNumber,
-      :result => native.getResult.toString,
-      :status_message => native.getBuildStatusSummary.message,
-      :time => time,
-      :url => native.getUrl
-    }
-  end
 end

spec/models/notes_generator_spec.rb → spec/lib/build_notes_spec.rb

@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe NotesGenerator do
+describe BuildNotes do
   let(:launcher) { stub }
   let(:listener) { stub(:info => true) }
   let(:native) do

spec/models/builder_spec.rb → spec/lib/build_participant_spec.rb

@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Builder do
+describe BuildParticipant do
   context 'a build executor' do
     let(:listener) { stub(:info => true) }
     let(:launcher) { stub(:execute) }
@@ -12,10 +12,10 @@
     let(:build) { stub(:workspace => workspace) }
 
     subject {
-      class MyBuilder
-        include Builder
+      class MyBuildParticipant
+        include BuildParticipant
       end
-      MyBuilder.new
+      MyBuildParticipant.new
     }
 
     before do
@@ -26,10 +26,28 @@ class MyBuilder
       BuildContext.instance.unset
     end
 
+    context '.build' do
+      it 'returns the build set in the context singleton' do
+        subject.build == build
+      end
+    end
+
+    context '.listener' do
+      it 'returns the listener set in the context singleton' do
+        subject.listener == listener
+      end
+    end
+
+    context '.launcher' do
+      it 'returns the launcher set in the context singleton' do
+        subject.launcher == launcher
+      end
+    end
+
     context '.run' do
       let(:out) { StringIO.new << "stdout" }
       let(:err) { StringIO.new << "stderr" }
-      
+
       before do
         launcher.should_receive(:execute).and_return(1)
       end

spec/models/git_updater_spec.rb → spec/lib/git_updater_spec.rb

@@ -33,18 +33,6 @@
       BuildContext.instance.unset
     end
 
-    context '.update!' do
-      it 'does not raise when push succeeds' do
-        subject.stub(:run => {:val => 0, :out => ''})
-        lambda { subject.update! }.should_not raise_error
-      end
-
-      it 'raises when push fails' do
-        subject.stub(:run => {:val => 1})
-        subject.update!.should raise_error
-      end
-    end
-
     context '.fetch_notes' do
       it 'executes a command to fetch the latest notes' do
         subject.should_receive(:run)
@@ -70,10 +58,19 @@
 
     context '.push_notes' do
       it 'adds and pushes the note' do
-        subject.should_receive(:run).twice
-        subject.push_notes
+        subject.should_receive(:run).twice.and_return({:val => 0})
+        subject.push_notes("a note")
+      end
+
+      it 'does not raise when push succeeds' do
+        subject.stub(:run => {:val => 0, :out => ''})
+        lambda { subject.push_notes("a note") }.should_not raise_error
       end
-    end
 
+      it 'raises when push fails' do
+        subject.stub(:run => {:val => 1})
+        lambda { subject.push_notes("a note") }.should raise_error GitUpdater::ConcurrentUpdateError
+      end
+    end
   end
 end

spec/models/git_notes_publisher_spec.rb

@@ -8,6 +8,7 @@
     let(:git_updater) { stub }
 
     before do
+      BuildNotes.stub(:new => stub(:notes => {}))
       BuildContext.instance.set(build, launcher, listener)
     end
 
@@ -26,32 +27,10 @@
       end
 
       it 'tries to update a note three times in the case of failure' do
-        git_updater.should_receive(:update!).exactly(3).times.and_raise(ConcurrentUpdateError)
+        git_updater.should_receive(:update!).exactly(3).times.and_raise(GitUpdater::ConcurrentUpdateError)
         listener.should_receive(:warn).exactly(2).times
-        lambda { subject.perform(build, launcher, listener) }.should raise_error(ConcurrentUpdateError)
+        lambda { subject.perform(build, launcher, listener) }.should raise_error(GitUpdater::ConcurrentUpdateError)
       end
     end
-
-    context '.build_note_hash' do
-      let(:time) { Time.now }
-      let(:native) do
-        stub({
-          :getBuiltOnStr => 'master',
-          :getTimeInMillis => time.to_i * 1000,
-          :getFullDisplayName => 'project-master #951',
-          :getId => '2012-04-10_20-52-03',
-          :getNumber => '951',
-          :getResult => stub(:toString => 'SUCCESS'),
-          :getBuildStatusSummary => stub(:message => 'stable'),
-          :getUrl => 'job/project-master/951'
-        })
-      end
-      let(:build) { stub(:send => native) }
-
-      it 'returns a jenkins build note' do
-        subject.send(:build_note_hash, build).should_not be_nil
-      end
-    end
-
   end
 end

spec/spec_helper.rb

@@ -14,10 +14,4 @@ def self.display_name(str) ; end
 end
 
 # Classes defined in this plugin
-require 'build_context'
-require 'builder'
-require 'concurrent_update_error'
-require 'constants'
 require 'git_notes_publisher'
-require 'git_updater'
-require 'notes_generator'