Spec::JUnitFormatter enhancements (#8599)

* Forward arguments to formatter finish method

* Report elapsed time in Spec::JUnitFormatter

* Report number of pending specs (as “disabled”) in Spec::JUnitFormatter

* Output errored class name as an “error[type]” attribute

* Remove redundant `begin … end`

* Output hostname for testsuite in Spec::JUnitFormatter

* Move `#chomp` call to the helper

* Output timestamp for testsuite in Spec::JUnitFormatter

* Correctly report pending testcases as “skipped”

* Strip current working directory from reported “testcase[classname]” attribute

* Remove timestamp attribute mocking in specs since it ain’t test anything rly

* Add specs for Spec::JUnitFormatter#classname

* Consistently use “skipped”

* Use .total_seconds for moar clarity

* Escape control characters for “testcase[name]”

* Add `—junit_output_path` for more granular control of the junit output location

* Change `—junit_output` to take full path to the junit xml file

* Properly mock `Spec::JUnitFormatter#started_at`

Makefile

@@ -21,7 +21,7 @@ progress ?=     ## Enable progress output
 threads ?=      ## Maximum number of threads to use
 debug ?=        ## Add symbolic debug info
 verbose ?=      ## Run specs in verbose mode
-junit_output ?= ## Directory to output junit results
+junit_output ?= ## Path to output junit results
 static ?=       ## Enable static linking
 
 O := .build

spec/std/spec/junit_formatter_spec.cr

@@ -1,92 +1,137 @@
 require "spec"
 require "xml"
 
+class Spec::JUnitFormatter
+  property started_at
+end
+
 describe "JUnit Formatter" do
   it "reports successful results" do
-    output = build_report do |f|
+    output = build_report_with_no_timestamp do |f|
       f.report Spec::Result.new(:success, "should do something", "spec/some_spec.cr", 33, nil, nil)
       f.report Spec::Result.new(:success, "should do something else", "spec/some_spec.cr", 50, nil, nil)
     end
 
     expected = <<-XML
                  <?xml version="1.0"?>
-                 <testsuite tests="2" errors="0" failures="0">
+                 <testsuite tests="2" skipped="0" errors="0" failures="0" time="0.0" hostname="#{System.hostname}">
                    <testcase file="spec/some_spec.cr" classname="spec.some_spec" name="should do something"/>
                    <testcase file="spec/some_spec.cr" classname="spec.some_spec" name="should do something else"/>
                  </testsuite>
                  XML
 
-    output.chomp.should eq(expected)
+    output.should eq(expected)
+  end
+
+  it "reports skipped" do
+    output = build_report_with_no_timestamp do |f|
+      f.report Spec::Result.new(:pending, "should do something", "spec/some_spec.cr", 33, nil, nil)
+    end
+
+    expected = <<-XML
+                 <?xml version="1.0"?>
+                 <testsuite tests="1" skipped="1" errors="0" failures="0" time="0.0" hostname="#{System.hostname}">
+                   <testcase file="spec/some_spec.cr" classname="spec.some_spec" name="should do something">
+                     <skipped/>
+                   </testcase>
+                 </testsuite>
+                 XML
+
+    output.should eq(expected)
   end
 
   it "reports failures" do
-    output = build_report do |f|
+    output = build_report_with_no_timestamp do |f|
       f.report Spec::Result.new(:fail, "should do something", "spec/some_spec.cr", 33, nil, nil)
     end
 
     expected = <<-XML
                  <?xml version="1.0"?>
-                 <testsuite tests="1" errors="0" failures="1">
+                 <testsuite tests="1" skipped="0" errors="0" failures="1" time="0.0" hostname="#{System.hostname}">
                    <testcase file="spec/some_spec.cr" classname="spec.some_spec" name="should do something">
                      <failure/>
                    </testcase>
                  </testsuite>
                  XML
 
-    output.chomp.should eq(expected)
+    output.should eq(expected)
   end
 
   it "reports errors" do
-    output = build_report do |f|
+    output = build_report_with_no_timestamp do |f|
       f.report Spec::Result.new(:error, "should do something", "spec/some_spec.cr", 33, nil, nil)
     end
 
     expected = <<-XML
                  <?xml version="1.0"?>
-                 <testsuite tests="1" errors="1" failures="0">
+                 <testsuite tests="1" skipped="0" errors="1" failures="0" time="0.0" hostname="#{System.hostname}">
                    <testcase file="spec/some_spec.cr" classname="spec.some_spec" name="should do something">
                      <error/>
                    </testcase>
                  </testsuite>
                  XML
 
-    output.chomp.should eq(expected)
+    output.should eq(expected)
   end
 
   it "reports mixed results" do
-    output = build_report do |f|
+    output = build_report_with_no_timestamp do |f|
       f.report Spec::Result.new(:success, "should do something1", "spec/some_spec.cr", 33, 2.seconds, nil)
       f.report Spec::Result.new(:fail, "should do something2", "spec/some_spec.cr", 50, 0.5.seconds, nil)
       f.report Spec::Result.new(:error, "should do something3", "spec/some_spec.cr", 65, nil, nil)
-      f.report Spec::Result.new(:error, "should do something4", "spec/some_spec.cr", 80, nil, nil)
+      f.report Spec::Result.new(:pending, "should do something4", "spec/some_spec.cr", 80, nil, nil)
     end
 
     expected = <<-XML
                  <?xml version="1.0"?>
-                 <testsuite tests="4" errors="2" failures="1">
-                   <testcase file="spec/some_spec.cr" classname="spec.some_spec" name="should do something1"/>
-                   <testcase file="spec/some_spec.cr" classname="spec.some_spec" name="should do something2">
+                 <testsuite tests="4" skipped="1" errors="1" failures="1" time="0.0" hostname="#{System.hostname}">
+                   <testcase file="spec/some_spec.cr" classname="spec.some_spec" name="should do something1" time="2.0"/>
+                   <testcase file="spec/some_spec.cr" classname="spec.some_spec" name="should do something2" time="0.5">
                      <failure/>
                    </testcase>
                    <testcase file="spec/some_spec.cr" classname="spec.some_spec" name="should do something3">
                      <error/>
                    </testcase>
                    <testcase file="spec/some_spec.cr" classname="spec.some_spec" name="should do something4">
-                     <error/>
+                     <skipped/>
                    </testcase>
                  </testsuite>
                  XML
 
-    output.chomp.should eq(expected)
+    output.should eq(expected)
+  end
+
+  it "encodes class names from the relative file path" do
+    output = build_report do |f|
+      f.report Spec::Result.new(:success, "foo", __FILE__, __LINE__, nil, nil)
+    end
+
+    classname = XML.parse(output).xpath_string("string(//testsuite/testcase[1]/@classname)")
+    classname.should eq("spec.std.spec.junit_formatter_spec")
+  end
+
+  it "outputs timestamp according to RFC 3339" do
+    now = Time.utc
+
+    output = build_report(timestamp: now) do |f|
+      f.report Spec::Result.new(:success, "foo", __FILE__, __LINE__, nil, nil)
+    end
+
+    classname = XML.parse(output).xpath_string("string(//testsuite[1]/@timestamp)")
+    classname.should eq(now.to_rfc3339)
   end
 
   it "escapes spec names" do
     output = build_report do |f|
       f.report Spec::Result.new(:success, %(complicated " <n>'&ame), __FILE__, __LINE__, nil, nil)
+      f.report Spec::Result.new(:success, %(ctrl characters follow - \r\n), __FILE__, __LINE__, nil, nil)
     end
 
     name = XML.parse(output).xpath_string("string(//testsuite/testcase[1]/@name)")
     name.should eq(%(complicated \" <n>'&ame))
+
+    name = XML.parse(output).xpath_string("string(//testsuite/testcase[2]/@name)")
+    name.should eq(%(ctrl characters follow - \\r\\n))
   end
 
   it "report failure stacktrace if present" do
@@ -120,18 +165,24 @@ describe "JUnit Formatter" do
   end
 end
 
-private def build_report
+private def build_report(timestamp = nil)
   output = String::Builder.new
   formatter = Spec::JUnitFormatter.new(output)
+  formatter.started_at = timestamp if timestamp
   yield formatter
-  formatter.finish
-  output.to_s
+  formatter.finish(Time::Span.zero, false)
+  output.to_s.chomp
 end
 
-private def exception_with_backtrace(msg)
-  begin
-    raise Exception.new(msg)
-  rescue e
-    e
+private def build_report_with_no_timestamp
+  output = build_report do |formatter|
+    yield formatter
   end
+  output.gsub(/\s*timestamp="(.+?)"/, "")
+end
+
+private def exception_with_backtrace(msg)
+  raise Exception.new(msg)
+rescue e
+  e
 end

spec/std/spec/tap_formatter_spec.cr

@@ -5,7 +5,7 @@ private def build_report
   String.build do |io|
     formatter = Spec::TAPFormatter.new(io)
     yield formatter
-    formatter.finish
+    formatter.finish(Time::Span.zero, false)
   end
 end
 

src/spec.cr

@@ -129,8 +129,8 @@ OptionParser.parse do |opts|
       abort("order must be either 'default', 'random', or a numeric seed value")
     end
   end
-  opts.on("--junit_output OUTPUT_DIR", "generate JUnit XML output") do |output_dir|
-    junit_formatter = Spec::JUnitFormatter.file(output_dir)
+  opts.on("--junit_output OUTPUT_PATH", "generate JUnit XML output within the given OUTPUT_PATH") do |output_path|
+    junit_formatter = Spec::JUnitFormatter.file(Path.new(output_path))
     Spec.add_formatter(junit_formatter)
   end
   opts.on("--help", "show this help") do |pattern|

src/spec/context.cr

@@ -65,7 +65,7 @@ module Spec
     end
 
     def finish(elapsed_time, aborted = false)
-      Spec.formatters.each(&.finish)
+      Spec.formatters.each(&.finish(elapsed_time, aborted))
       Spec.formatters.each(&.print_results(elapsed_time, aborted))
     end
 

src/spec/example.cr

@@ -45,19 +45,17 @@ module Spec
     end
 
     private def internal_run(start, block)
-      begin
-        @parent.run_before_each_hooks
-        block.call
-        @parent.report(:success, description, file, line, Time.monotonic - start)
-      rescue ex : Spec::AssertionFailed
-        @parent.report(:fail, description, file, line, Time.monotonic - start, ex)
-        Spec.abort! if Spec.fail_fast?
-      rescue ex
-        @parent.report(:error, description, file, line, Time.monotonic - start, ex)
-        Spec.abort! if Spec.fail_fast?
-      ensure
-        @parent.run_after_each_hooks
-      end
+      @parent.run_before_each_hooks
+      block.call
+      @parent.report(:success, description, file, line, Time.monotonic - start)
+    rescue ex : Spec::AssertionFailed
+      @parent.report(:fail, description, file, line, Time.monotonic - start, ex)
+      Spec.abort! if Spec.fail_fast?
+    rescue ex
+      @parent.report(:error, description, file, line, Time.monotonic - start, ex)
+      Spec.abort! if Spec.fail_fast?
+    ensure
+      @parent.run_after_each_hooks
     end
   end
 end

src/spec/formatter.cr

@@ -16,7 +16,7 @@ module Spec
     def report(result)
     end
 
-    def finish
+    def finish(elapsed_time, aborted)
     end
 
     def print_results(elapsed_time : Time::Span, aborted : Bool)
@@ -30,7 +30,7 @@ module Spec
       @io.flush
     end
 
-    def finish
+    def finish(elapsed_time, aborted)
       @io.puts
     end
 

src/spec/junit_formatter.cr

@@ -3,6 +3,8 @@ require "html"
 module Spec
   # :nodoc:
   class JUnitFormatter < Formatter
+    @started_at = Time.utc
+
     @results = [] of Spec::Result
     @summary = {} of Symbol => Int32
 
@@ -12,12 +14,17 @@ module Spec
       @results << result
     end
 
-    def finish
+    def finish(elapsed_time, aborted)
       io = @io
       io.puts %(<?xml version="1.0"?>)
       io << %(<testsuite tests=") << @results.size
+      io << %(" skipped=") << (@summary[:pending]? || 0)
       io << %(" errors=") << (@summary[:error]? || 0)
-      io << %(" failures=") << (@summary[:fail]? || 0) << %(">)
+      io << %(" failures=") << (@summary[:fail]? || 0)
+      io << %(" time=") << elapsed_time.total_seconds
+      io << %(" timestamp=") << @started_at.to_rfc3339
+      io << %(" hostname=") << System.hostname
+      io << %(">)
 
       io.puts
 
@@ -27,26 +34,45 @@ module Spec
       io.close
     end
 
-    def self.file(output_dir)
-      Dir.mkdir_p(output_dir)
-      output_file_path = File.join(output_dir, "output.xml")
-      file = File.new(output_file_path, "w")
+    def self.file(output_path : Path)
+      Dir.mkdir_p(output_path.dirname)
+      file = File.new(output_path, "w")
       JUnitFormatter.new(file)
     end
 
+    private def escape_xml_attr(value)
+      String.build do |io|
+        reader = Char::Reader.new(value)
+        while reader.has_next?
+          case current_char = reader.current_char
+          when .control?
+            current_char.to_s.inspect_unquoted(io)
+          else
+            current_char.to_s(io)
+          end
+          reader.next_char
+        end
+      end
+    end
+
     # -------- private utility methods
     private def write_report(result, io)
       io << %(  <testcase file=")
       HTML.escape(result.file, io)
       io << %(" classname=")
       HTML.escape(classname(result), io)
       io << %(" name=")
-      HTML.escape(result.description, io)
+      HTML.escape(escape_xml_attr(result.description), io)
+
+      if elapsed = result.elapsed
+        io << %(" time=")
+        io << elapsed.total_seconds
+      end
 
       if tag = inner_content_tag(result.kind)
         io.puts %(">)
 
-        if exception = result.exception
+        if (exception = result.exception) && result.kind != :pending
           write_inner_content(tag, exception, io)
         else
           io << "    <" << tag << "/>\n"
@@ -59,8 +85,9 @@ module Spec
 
     private def inner_content_tag(kind)
       case kind
-      when :error then "error"
-      when :fail  then "failure"
+      when :error   then "error"
+      when :fail    then "failure"
+      when :pending then "skipped"
       end
     end
 
@@ -72,6 +99,11 @@ module Spec
         HTML.escape(message, io)
         io << '"'
       end
+      if tag == :error
+        io << %( type=")
+        io << exception.class.name
+        io << '"'
+      end
       io << '>'
 
       if backtrace = exception.backtrace?
@@ -82,7 +114,12 @@ module Spec
     end
 
     private def classname(result)
-      result.file.sub(%r{\.[^/.]+\Z}, "").gsub("/", ".").gsub(/\A\.+|\.+\Z/, "")
+      path = Path[result.file].expand
+      path.to_s
+        .lchop(Dir.current)
+        .rchop(path.extension)
+        .gsub(File::SEPARATOR, '.')
+        .strip('.')
     end
   end
 end