Don't use rexml for the JUnit formatter

Closes #371 Follows rubocop/rubocop#13165 This leaves the output mostly the same, except that text is no longer wrapped in CDATA. Some things possibly need to be escaped now, and CDATA has different escaping needs. So, just use the same escaping everywhere
Shopify · Sep 18, 2024 · 12e1659 · 12e1659
1 parent c502d90
commit 12e1659
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 103 deletions.
diff --git a/Gemfile.lock b/Gemfile.lock
@@ -60,7 +60,7 @@ GEM
       mini_portile2 (~> 2.8.2)
       racc (~> 1.4)
     parallel (1.24.0)
-    parser (3.3.0.5)
+    parser (3.3.5.0)
       ast (~> 2.4.1)
       racc
     racc (1.7.3)
@@ -74,8 +74,6 @@ GEM
     rainbow (3.1.1)
     rake (13.0.6)
     regexp_parser (2.9.0)
-    rexml (3.3.6)
-      strscan
     rspec (3.11.0)
       rspec-core (~> 3.11.0)
       rspec-expectations (~> 3.11.0)
@@ -89,24 +87,22 @@ GEM
       diff-lcs (>= 1.2.0, < 2.0)
       rspec-support (~> 3.11.0)
     rspec-support (3.11.0)
-    rubocop (1.62.1)
+    rubocop (1.66.1)
       json (~> 2.3)
       language_server-protocol (>= 3.17.0)
       parallel (~> 1.10)
       parser (>= 3.3.0.2)
       rainbow (>= 2.2.2, < 4.0)
-      regexp_parser (>= 1.8, < 3.0)
-      rexml (>= 3.2.5, < 4.0)
-      rubocop-ast (>= 1.31.1, < 2.0)
+      regexp_parser (>= 2.4, < 3.0)
+      rubocop-ast (>= 1.32.2, < 2.0)
       ruby-progressbar (~> 1.7)
       unicode-display_width (>= 2.4.0, < 3.0)
-    rubocop-ast (1.31.2)
-      parser (>= 3.3.0.4)
+    rubocop-ast (1.32.3)
+      parser (>= 3.3.1.0)
     rubocop-shopify (2.15.1)
       rubocop (~> 1.51)
     ruby-progressbar (1.13.0)
     smart_properties (1.17.0)
-    strscan (3.1.0)
     tzinfo (2.0.6)
       concurrent-ruby (~> 1.0)
     unicode-display_width (2.5.0)

diff --git a/lib/erb_lint/reporters/junit_reporter.rb b/lib/erb_lint/reporters/junit_reporter.rb
@@ -1,111 +1,62 @@
 # frozen_string_literal: true
 
-require "rexml/document"
-require "rexml/formatters/pretty"
-
 module ERBLint
   module Reporters
     class JunitReporter < Reporter
+      ESCAPE_MAP = {
+        '"' => "&quot;",
+        "'" => "&apos;",
+        "<" => "&lt;",
+        ">" => "&gt;",
+        "&" => "&amp;",
+      }.freeze
+
+      PROPERTIES = [
+        ["erb_lint_version", ERBLint::VERSION],
+        ["ruby_engine", RUBY_ENGINE],
+        ["ruby_version", RUBY_VERSION],
+        ["ruby_patchlevel", RUBY_PATCHLEVEL.to_s],
+        ["ruby_platform", RUBY_PLATFORM],
+      ].freeze
+
       def preview; end
 
       def show
-        xml = create_junit_xml
-        formatted_xml_string = StringIO.new
-        REXML::Formatters::Pretty.new.write(xml, formatted_xml_string)
-        puts formatted_xml_string.string
-      end
-
-      private
-
-      CONTEXT = {
-        prologue_quote: :quote,
-        attribute_quote: :quote,
-      }
-
-      def create_junit_xml
-        # create prologue
-        xml = REXML::Document.new(nil, CONTEXT)
-        xml << REXML::XMLDecl.new("1.0", "UTF-8")
-
-        xml.add_element(create_testsuite_element)
+        puts %(<?xml version="1.0" encoding="UTF-8"?>)
+        puts %(<testsuite name="erblint" tests="#{@stats.processed_files.size}" failures="#{@stats.found}">)
 
-        xml
-      end
-
-      def create_testsuite_element
-        tests = stats.processed_files.size
-        failures = stats.found
-        testsuite_element = REXML::Element.new("testsuite", nil, CONTEXT)
-        testsuite_element.add_attribute("name", "erblint")
-        testsuite_element.add_attribute("tests", tests.to_s)
-        testsuite_element.add_attribute("failures", failures.to_s)
-
-        testsuite_element.add_element(create_properties)
+        puts %(  <properties>)
+        PROPERTIES.each do |key, value|
+          puts %(    <property name="#{xml_escape(key)}" value="#{xml_escape(value)}"/>)
+        end
+        puts %(  </properties>)
 
         processed_files.each do |filename, offenses|
+          filename_escaped = xml_escape(filename)
           if offenses.empty?
-            testcase_element = REXML::Element.new("testcase", nil, CONTEXT)
-            testcase_element.add_attribute("name", filename.to_s)
-            testcase_element.add_attribute("file", filename.to_s)
-
-            testsuite_element.add_element(testcase_element)
-          end
-
-          offenses.each do |offense|
-            testsuite_element.add_element(create_testcase(filename, offense))
+            puts %(  <testcase name="#{filename_escaped}" file="#{filename_escaped}"/>)
+          else
+            offenses.each do |offense|
+              type = offense.simple_name
+              message = "#{type}: #{offense.message}"
+              body = "#{message} at #{filename}:#{offense.line_number}:#{offense.column}"
+
+              puts %(  <testcase name="#{filename_escaped}" file="#{filename_escaped}" lineno="#{offense.line_number}">)
+              puts %(    <failure message="#{xml_escape(message)}" type="#{xml_escape(type)}">)
+              puts %(      #{xml_escape(body)})
+              puts %(    </failure>)
+              puts %(  </testcase>)
+            end
           end
         end
 
-        testsuite_element
+        puts %(</testsuite>)
       end
 
-      def create_properties
-        properties_element = REXML::Element.new("properties", nil, CONTEXT)
-
-        [
-          ["erb_lint_version", ERBLint::VERSION],
-          ["ruby_engine", RUBY_ENGINE],
-          ["ruby_version", RUBY_VERSION],
-          ["ruby_patchlevel", RUBY_PATCHLEVEL.to_s],
-          ["ruby_platform", RUBY_PLATFORM],
-        ].each do |property_attribute|
-          properties_element.add_element(create_property(*property_attribute))
-        end
-
-        properties_element
-      end
-
-      def create_property(name, value)
-        property_element = REXML::Element.new("property")
-        property_element.add_attribute("name", name)
-        property_element.add_attribute("value", value)
-
-        property_element
-      end
-
-      def create_testcase(filename, offense)
-        testcase_element = REXML::Element.new("testcase", nil, CONTEXT)
-        testcase_element.add_attribute("name", filename.to_s)
-        testcase_element.add_attribute("file", filename.to_s)
-        testcase_element.add_attribute("lineno", offense.line_number.to_s)
-
-        testcase_element.add_element(create_failure(filename, offense))
-
-        testcase_element
-      end
-
-      def create_failure(filename, offense)
-        message = offense.message
-        type = offense.simple_name
-
-        failure_element = REXML::Element.new("failure", nil, CONTEXT)
-        failure_element.add_attribute("message", "#{type}: #{message}")
-        failure_element.add_attribute("type", type.to_s)
-
-        cdata_element = REXML::CData.new("#{type}: #{message} at #{filename}:#{offense.line_number}:#{offense.column}")
-        failure_element.add_text(cdata_element)
+      private
 
-        failure_element
+      def xml_escape(string)
+        string.gsub(Regexp.union(ESCAPE_MAP.keys), ESCAPE_MAP)
       end
     end
   end

diff --git a/spec/erb_lint/cli_spec.rb b/spec/erb_lint/cli_spec.rb
@@ -4,7 +4,6 @@
 require "spec_utils"
 require "erb_lint/cli"
 require "erb_lint/cache"
-require "pp"
 require "fakefs"
 require "fakefs/spec_helpers"
 

diff --git a/spec/erb_lint/fixtures/junit.xml b/spec/erb_lint/fixtures/junit.xml
@@ -9,12 +9,12 @@
   </properties>
   <testcase name="app/views/subscriptions/_loader.html.erb" file="app/views/subscriptions/_loader.html.erb" lineno="1">
     <failure message="SpaceInHtmlTag: Extra space detected where there should be no space." type="SpaceInHtmlTag">
-      <![CDATA[SpaceInHtmlTag: Extra space detected where there should be no space. at app/views/subscriptions/_loader.html.erb:1:7]]>
+      SpaceInHtmlTag: Extra space detected where there should be no space. at app/views/subscriptions/_loader.html.erb:1:7
     </failure>
   </testcase>
   <testcase name="app/views/subscriptions/_loader.html.erb" file="app/views/subscriptions/_loader.html.erb" lineno="52">
     <failure message="ClosingErbTagIndent: Remove newline before `%%&gt;` to match start of tag." type="ClosingErbTagIndent">
-      <![CDATA[ClosingErbTagIndent: Remove newline before `%%>` to match start of tag. at app/views/subscriptions/_loader.html.erb:52:10]]>
+      ClosingErbTagIndent: Remove newline before `%%&gt;` to match start of tag. at app/views/subscriptions/_loader.html.erb:52:10
     </failure>
   </testcase>
   <testcase name="app/views/application/index.html.erb" file="app/views/application/index.html.erb"/>