Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If the failure message or stack contains colors, the XML will render invalid #170

Merged
merged 3 commits into from
Nov 24, 2017

Conversation

kraenhansen
Copy link
Contributor

@kraenhansen kraenhansen commented Jul 19, 2017

The attached file was produced with the JUnitXmlReporter: test-results.xml.zip

It is invalid XML as a <![CDATA[ blob contains the 0x1B byte on line 1045 at column 71, which is not proper UTF-8.

This PR introduces escapeControlChars which is a less agressive escape function that is now used by escapeInvalidXmlChars after escaping <, />, etc.

escapeControlChars now wraps the failure.stack || failure.message to get rid of the unwanted bytes.

function escapeInvalidXmlChars(str) {
return str.replace(/\&/g, "&amp;")
const escaped = str.replace(/\&/g, "&amp;")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must not use const in any of the reporter source files, as it may be used on a browser version that does not support const—this should be var for maximum compatibility. New keywords are fine to use in the spec files (as they only run in node), but reporter files need to be much more backwards compatible.

I am happy to merge this PR if it is updated to use var.

Thanks.

Copy link
Collaborator

@putermancer putermancer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must not use const in any of the reporter source files, as it may be used on a browser version that does not support const—this should be var for maximum compatibility. New keywords are fine to use in the spec files (as they only run in node), but reporter files need to be much more backwards compatible.

I am happy to merge this PR if it is updated to use var.

(Sorry for the double entry, this is my first time using the "Review changes" feature.)

@kraenhansen
Copy link
Contributor Author

@bloveridge Changed const to a var 👍

@putermancer
Copy link
Collaborator

Thanks!

@putermancer putermancer merged commit 4218fb7 into larrymyers:master Nov 24, 2017
@kraenhansen kraenhansen deleted the escape-xml-more branch November 24, 2017 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants