Remove invalid ESC control sequence from XML output

[stevenxu-db]

This change forcibly deletes any occurrences of the ESC control sequence in the output XML file.

RTL emits the ESC control sequence \u001b or \x1B sometimes, and the xml package doesn't escape or exclude it. This is not valid XML and causes some parsers, including the one used in this package's unit tests, to fail. #197 was a previous report of this.

Without this fix, the newly added test fails with an XML parse error.

We could also fix in xml but that project appears less actively maintained, and given nobody has reported this it may not be a widespread problem worth messing with.

This also fixes leaking state between some tests.

[amirgalor-gong]

Would be great if this can be reviewed and possibly merged - i'm seeing some flaky-ness using this :(

[stevenxu-db]

Here's our patch on 14.0.1 while we're awaiting upstream review and fix/release.

diff --git a/index.js b/index.js
index 13b7394b622396e6357eb318c1a71fc8d7db27f7..a80595e98b40d4edf62b4ae6b51c268e1bdcc9fe 100644
--- a/index.js
+++ b/index.js
@@ -34,8 +34,12 @@ const processor = (report, reporterOptions = {}, jestRootDir = null) => {
   // Ensure output path exists
   mkdirp.sync(path.dirname(outputPath));

+  // Clean ESC character, which is invalid XML and some libraries include in error messages
+  const xmlString = xml(jsonResults, { indent: '  ', declaration: true });
+  const cleanedXmlString = xmlString.replace(/\u001b/g, '');
+
   // Write data to file
-  fs.writeFileSync(outputPath, xml(jsonResults, { indent: '  ', declaration: true }));
+  fs.writeFileSync(outputPath, cleanedXmlString);

   // Jest 18 compatibility
   return report;

[palmerj3]

Sorry for the delay reviewing this. I was traveling for work and then was ill.

I left a comment for you to review.

I would like to avoid simple string replacements and try to utilize strip-ansi or something similar. I already have logic that attempts to remove these characters from failure tags so perhaps that logic needs to be adjusted.

[palmerj3]

Instead of doing a simple string replace can you see if you can utilize something more generic like strip-ansi?

We use that package when generating the failure tag. So perhaps it needs to be included here or tweaked.

See https://github.com/jest-community/jest-junit/blob/master/utils/buildJsonResults.js#L77

[stevenxu-db]

Thank you for the pointer. I've collocated my regexp replace with the existing function.

strip-ansi is already operating on the offending string, but it appears that strip-ansi strips only ANSI escape codes, which are ESC plus some characters. But it doesn't also strip plain ASCII control characters like ESC, which are illegal in the XML spec.

I failed to find a suitable package on the npm registry. I did find a snippet https://gist.github.com/john-doherty/b9195065884cdbfd2017a4756e6409cc from @john-doherty that appears to be more robust than my naive approach. I initially wanted to keep the scope of the change small, but if you prefer, I can inline a copy with attribution or work with the author to publish it as a separate package with appropriate license. @palmerj3, please just let me know as I'm happy to follow your preference.

[palmerj3]

Cool thanks for clarifying. Your implementation looks fine to me so let's roll with that.

Running the CI job now. If all looks good I'll merge and cut a release in the morning.

[stevenxu-db]

It looks like the checks passed.

Sorry for the ping. It's not urgent for us since we've already patched locally, but it looks like there's one other PR that intersects and a couple of downstream projects that could use this fix. Thanks for your attention here and hope you're doing better!

[palmerj3]

Thanks for the hard work!

This is now published in jest-junit 15.0.0