Escape ]]> in xunit xml output

[arichardson]

Summary

This sequence ends the CDATA block so any characters after that are no
longer escaped. This can be fixed by replacing "]]>" with "]]]]><![CDATA[>".

[cmatthews]

Is this an issue you are hitting? I noticed we spend a lot of time in string replace while writing the xunit files when the output is large (for instance if every test in the test suite fails). I was hoping the number of replaces we do could go down.

Besides that LGTM.

[delcypher]

utils/lit/tests/xunit-output.py

2 ↗

(On Diff #146849)

@cmatthews
Wait. I know this isn't part of the patch but what's that || true doing there? That's ignoring the exit code of lit. Do we really want to do that?

[cmatthews]

There is a test case that intentionally fails to provide failing output. That makes lit return an error exit value. Any way to change what lit exits with?

[arichardson]

In D46886#1099773, @cmatthews wrote:

Is this an issue you are hitting? I noticed we spend a lot of time in string replace while writing the xunit files when the output is large (for instance if every test in the test suite fails). I was hoping the number of replaces we do could go down.

Besides that LGTM.

I currently don't have a test that triggers this behaviour but I believe I originally did hit this when I first implemented this escaping. Will python return a new string if the ]]> is not included in the input? If not I doubt this adds that much overhead compared to the tons of replacements that were needed to replace all XML chars before.

Regarding the || true: I guess that should just be not llvm-lit instead?

[delcypher]

In D46886#1099798, @cmatthews wrote:

There is a test case that intentionally fails to provide failing output. That makes lit return an error exit value. Any way to change what lit exits with?

Not that I know of. But if you're expecting lit to return a non zero exit code should you run not %{lit} ... instead?

[cmatthews]

The test does not care what the return code of lit is, just that the xml output is correct.

[delcypher]

In D46886#1099816, @cmatthews wrote:

The test does not care what the return code of lit is, just that the xml output is correct.

That seems a little risky to me. The lit command could fail to execute and you could pick up an old copy of %t.xunit.xml from a previously passing run because AFAIK %t.xunit.xml expands to something deterministic.
Because the exit code is completely ignored we would definitely miss this, where-as if the exit code was checked we would be less likely (but not guaranteed) to miss this.
In reality we probably ought to rm -f %t.xunit.xml first before running the test.

[cmatthews]

Sure, I will update the test today.

llvm/trunk/utils/lit/lit/Test.py

def writeJUnitXML(self, fil):
         testcase_xml = testcase_template.format(class_name=class_name, test_name=test_name, time=elapsed_time)
         fil.write(testcase_xml)
         if self.result.code.isFailure:
             fil.write(">\n\t<failure ><![CDATA[")
             if type(self.result.output) == unicode:
                 encoded_output = self.result.output.encode("utf-8", 'ignore')
             else:
                 encoded_output = self.result.output
-            fil.write(encoded_output)
+            # In the unlikely case that the output contains the CDATA terminator
+            # we wrap it by creating a new CDATA block
+            fil.write(encoded_output.replace("]]>", "]]]]><![CDATA[>"))
             fil.write("]]></failure>\n</testcase>")
         elif self.result.code == UNSUPPORTED:
             unsupported_features = self.getMissingRequiredFeatures()
             if unsupported_features:
                 skip_message = escape("Skipping because of: " + ", ".join(unsupported_features))
             else:
                 skip_message = "Skipping because of configuration."
 
             fil.write(">\n\t<skipped message=\"{}\" />\n</testcase>\n".format(skip_message))
         else:
             fil.write("/>")

llvm/trunk/utils/lit/tests/Inputs/xunit-output/bad&name.ini

 [global]
 result_code = FAIL
-result_output = & < > "
+result_output = & < > ]]> &"
 
 [results]
 value0 = 1
-value1 = 2.3456
+value1 = 2.3456
 

llvm/trunk/utils/lit/tests/xunit-output.py

 # Check xunit output
 # RUN: rm -rf %t.xunit.xml
 # RUN: not %{lit} --xunit-xml-output %t.xunit.xml %{inputs}/xunit-output
+# If xmllint is installed verify that the generated xml is well-formed
+# RUN: sh -c 'if command -v xmllint 2>/dev/null; then xmllint --noout %t.xunit.xml; fi'
 # RUN: FileCheck < %t.xunit.xml %s
 
 # CHECK: <?xml version="1.0" encoding="UTF-8" ?>
 # CHECK: <testsuites>
 # CHECK: <testsuite name='test-data' tests='1' failures='1' skipped='0'>
 # CHECK: <testcase classname='test-data.test-data' name='bad&amp;name.ini' time='{{[0-1]}}.{{[0-9]+}}'>
-# CHECK-NEXT: <failure ><![CDATA[& < > "]]></failure>
+# CHECK-NEXT: <failure ><![CDATA[& < > ]]]]><![CDATA[> &"]]></failure>
 # CHECK: </testsuite>
 # CHECK: </testsuites>