Improve annotation messages. (#6)

* Improve annotation messages.

* Refactor formatting with delegator pattern

* Change relative e2e test to run on real file

* Do not require delegate

Co-authored-by: Stef Schenkelaars <[email protected]>

Author: reitermarkus

.github/workflows/ci.yml

@@ -35,7 +35,6 @@ jobs:
       - name: Run RSpec
         run: bundle exec rspec spec/rspec
   e2e:
-    continue-on-error: true
     runs-on: ubuntu-latest
     steps:
       - name: Check out code
@@ -56,5 +55,9 @@ jobs:
           gem install bundler:2.1.4 --no-doc
           bundle config path vendor/bundle
           bundle install --jobs 4 --retry 3
-      - name: Run RSpec
-        run: '! bundle exec rspec spec/integration'
+      - name: Run RSpec in GITHUB_WORKSPACE
+        run: '! bundle exec rspec spec/integration/failing_spec.rb'
+      - name: Run RSpec in sub-directory of GITHUB_WORKSPACE
+        run: |
+          cd spec/integration
+          bundle exec rspec relative_path/pending_spec.rb --require ../spec_helper --format RSpec::Github::Formatter

lib/rspec/github/formatter.rb

@@ -2,27 +2,23 @@
 
 require 'rspec/core'
 require 'rspec/core/formatters/base_formatter'
+require 'rspec/github/notification_decorator'
 
 module RSpec
   module Github
     class Formatter < RSpec::Core::Formatters::BaseFormatter
       RSpec::Core::Formatters.register self, :example_failed, :example_pending
 
-      def self.relative_path(path)
-        workspace = File.realpath(ENV.fetch('GITHUB_WORKSPACE', '.'))
-        File.realpath(path).delete_prefix("#{workspace}#{File::SEPARATOR}")
-      end
-
       def example_failed(failure)
-        file, line = failure.example.location.split(':')
-        file = self.class.relative_path(file)
-        output.puts "\n::error file=#{file},line=#{line}::#{failure.message_lines.join('%0A')}"
+        notification = NotificationDecorator.new(failure)
+
+        output.puts "\n::error file=#{notification.path},line=#{notification.line}::#{notification.annotation}"
       end
 
       def example_pending(pending)
-        file, line = pending.example.location.split(':')
-        file = self.class.relative_path(file)
-        output.puts "\n::warning file=#{file},line=#{line}::#{pending.example.full_description}"
+        notification = NotificationDecorator.new(pending)
+
+        output.puts "\n::warning file=#{notification.path},line=#{notification.line}::#{notification.annotation}"
       end
     end
   end

lib/rspec/github/notification_decorator.rb

@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+module RSpec
+  module Github
+    class NotificationDecorator
+      # See https://github.community/t/set-output-truncates-multiline-strings/16852/3.
+      ESCAPE_MAP = {
+        '%' => '%25',
+        "\n" => '%0A',
+        "\r" => '%0D'
+      }.freeze
+
+      def initialize(notification)
+        @notification = notification
+      end
+
+      def line
+        example.location.split(':')[1]
+      end
+
+      def annotation
+        "#{example.full_description}\n\n#{message}"
+          .gsub(Regexp.union(ESCAPE_MAP.keys), ESCAPE_MAP)
+      end
+
+      def path
+        File.realpath(raw_path).delete_prefix("#{workspace}#{File::SEPARATOR}")
+      end
+
+      private
+
+      def example
+        @notification.example
+      end
+
+      def message
+        if @notification.respond_to?(:message_lines)
+          @notification.message_lines.join("\n")
+        else
+          "#{example.skip ? 'Skipped' : 'Pending'}: #{example.execution_result.pending_message}"
+        end
+      end
+
+      def raw_path
+        example.location.split(':')[0]
+      end
+
+      def workspace
+        File.realpath(ENV.fetch('GITHUB_WORKSPACE', '.'))
+      end
+    end
+  end
+end

spec/integration/failing_spec.rb

@@ -1,11 +1,20 @@
 # frozen_string_literal: true
 
-RSpec.describe RSpec::Github do
+RSpec.describe RSpec::Github::Formatter do
   it 'creates an error annotation for failing specs' do
     expect(true).to eq false
   end
 
-  it 'creates a warning annotation for pending specs'
+  it 'creates a warning annotation for unimplemented specs'
+
+  it 'creates a warning annotation for pending specs' do
+    pending 'because it is failing'
+    raise
+  end
+
+  it 'creates a warning annotation for skipped specs' do
+    skip 'because reasons'
+  end
 
   it 'does not create an annotiation for passing specs' do
     expect(true).to eq true

spec/integration/relative_path/pending_spec.rb

@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+RSpec.describe RSpec::Github::Formatter do
+  it 'adds annotiations correctly when running in a subdirectory'
+end

spec/rspec/github/formatter_spec.rb

@@ -6,11 +6,16 @@
   let(:output) { StringIO.new }
   let(:formatter) { described_class.new(output) }
   subject(:output_string) { output.string }
+  let(:skip) { false }
+
+  let(:location) { './spec/models/user_spec.rb:12' }
+
+  let(:pending_message) { 'Not yet implemented' }
 
   let(:execution_result) do
     double(
       'RSpec::Core::Example::ExecutionResult',
-      pending_message: 'Not yet implemented'
+      pending_message: pending_message
     )
   end
 
@@ -20,7 +25,8 @@
       execution_result: execution_result,
       full_description: 'User is expected to validate presence of name',
       description: 'is expected to validate presence of name',
-      location: './spec/models/user_spec.rb:12'
+      location: location,
+      skip: skip
     )
   end
 
@@ -30,53 +36,6 @@
                                      .and_return(File.join(Dir.pwd, 'spec/models/user_spec.rb'))
   end
 
-  describe '::relative_path' do
-    around do |example|
-      saved_github_workspace = ENV['GITHUB_WORKSPACE']
-      ENV['GITHUB_WORKSPACE'] = github_workspace
-
-      FileUtils.mkpath File.dirname(absolute_path)
-      FileUtils.touch absolute_path
-
-      Dir.chdir tmpdir do
-        example.run
-      end
-    ensure
-      FileUtils.rm_r tmpdir
-      ENV['GITHUB_WORKSPACE'] = saved_github_workspace
-    end
-
-    let(:tmpdir) { Dir.mktmpdir }
-    let(:relative_path) { 'this/is/a/relative_path.rb' }
-    let(:absolute_path) { File.join(tmpdir, relative_path) }
-
-    context 'if GITHUB_WORKSPACE is set' do
-      let(:github_workspace) { tmpdir }
-
-      it 'returns the path relative to it when already inside it' do
-        expect(described_class.relative_path('this/is/a/relative_path.rb')).to eq('this/is/a/relative_path.rb')
-      end
-
-      it 'returns the path relative to it when in a subdirectory of it' do
-        Dir.chdir 'this/is' do
-          expect(described_class.relative_path('a/relative_path.rb')).to eq('this/is/a/relative_path.rb')
-        end
-      end
-    end
-
-    context 'if GITHUB_WORKSPACE is unset' do
-      let(:github_workspace) { nil }
-
-      it 'returns the unchanged relative path' do
-        expect(described_class.relative_path('this/is/a/relative_path.rb')).to eq 'this/is/a/relative_path.rb'
-      end
-
-      it 'returns the relative path without a ./ prefix' do
-        expect(described_class.relative_path('./this/is/a/relative_path.rb')).to eq 'this/is/a/relative_path.rb'
-      end
-    end
-  end
-
   describe '#example_failed' do
     before { formatter.example_failed(notification) }
 
@@ -98,9 +57,52 @@
     it 'outputs the GitHub annotation formatted error' do
       is_expected.to eq <<~MESSAGE
 
-        ::error file=spec/models/user_spec.rb,line=12::#{notification.message_lines.join('%0A')}
+        ::error file=spec/models/user_spec.rb,line=12::#{example.full_description}%0A%0A#{notification.message_lines.join('%0A')}
       MESSAGE
     end
+
+    context 'relative_path to GITHUB_WORKSPACE' do
+      around do |example|
+        saved_github_workspace = ENV['GITHUB_WORKSPACE']
+        ENV['GITHUB_WORKSPACE'] = tmpdir
+
+        FileUtils.mkpath File.dirname(absolute_path)
+        FileUtils.touch absolute_path
+
+        Dir.chdir tmpdir do
+          example.run
+        end
+      ensure
+        FileUtils.rm_r tmpdir
+        ENV['GITHUB_WORKSPACE'] = saved_github_workspace
+      end
+
+      let(:tmpdir) { Dir.mktmpdir }
+      let(:relative_path) { 'this/is/a/relative_path.rb' }
+      let(:absolute_path) { File.join(tmpdir, relative_path) }
+
+      context 'inside root dir' do
+        let(:github_workspace) { tmpdir }
+        let(:location) { './this/is/a/relative_path.rb' }
+
+        it 'returns the relative path' do
+          is_expected.to include 'this/is/a/relative_path.rb'
+        end
+      end
+      context 'inside subdirectory dir' do
+        let(:github_workspace) { tmpdir }
+        let(:location) { './a/relative_path.rb' }
+        around do |example|
+          Dir.chdir 'this/is' do
+            example.run
+          end
+        end
+
+        it 'returns the relative path' do
+          is_expected.to include 'this/is/a/relative_path.rb'
+        end
+      end
+    end
   end
 
   describe '#example_pending' do
@@ -113,11 +115,24 @@
       )
     end
 
-    it 'outputs the GitHub annotation formatted error' do
-      is_expected.to eq <<~MESSAGE
+    context 'when pending' do
+      it 'outputs the GitHub annotation formatted warning' do
+        is_expected.to eq <<~MESSAGE
 
-        ::warning file=spec/models/user_spec.rb,line=12::#{example.full_description}
-      MESSAGE
+          ::warning file=spec/models/user_spec.rb,line=12::#{example.full_description}%0A%0APending: #{pending_message}
+        MESSAGE
+      end
+    end
+
+    context 'when skipped' do
+      let(:skip) { true }
+
+      it 'outputs the GitHub annotation formatted warning' do
+        is_expected.to eq <<~MESSAGE
+
+          ::warning file=spec/models/user_spec.rb,line=12::#{example.full_description}%0A%0ASkipped: #{pending_message}
+        MESSAGE
+      end
     end
   end
 end