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.

Sign up for GitHub

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

Jump to bottom

avoid double and tripple xUnit XML escaping #2178

Merged
merged 1 commit into from
May 23, 2016
Merged

avoid double and tripple xUnit XML escaping #2178

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 1 addition & 9 deletions lib/reporters/xunit.js
View file
Open in desktop
Expand Up @@ -130,7 +130,7 @@ XUnit.prototype.test = function(test) {

if (test.state === 'failed') {
var err = test.err;
this.write(tag('testcase', attrs, false, tag('failure', {}, false, cdata(escape(err.message) + '\n' + err.stack))));
this.write(tag('testcase', attrs, false, tag('failure', {}, false, escape(err.message) + '\n' + escape(err.stack))));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was used here, to triple escape the error message.

Copy link
Contributor

Choose a reason for hiding this comment

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

But then the output no longer has a surrounding '<![CDATA[..,]]>', right? Don't we need that so that special characters can be used in the error message and trace?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, okay, if they are escaped, it shouldn't be necessary. I wonder why it was used in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was double escaping from the first cut: https://github.com/mochajs/mocha/pull/179/files#diff-d80a115516f0cc2ec0b46a54c60077faR88

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I'm don't know much about this topic. I don't see:

  • Why (before your changes) the data was being escaped even though it was being put inside a CDATA tag. (which afaik is used to avoid having to escape chars)
  • Why escaping the data is preferred to just putting the data inside a CDATA tag. (since using the CDATA tag uses less bytes and makes the data readable by a human on a quick glance)

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be enough to stop using escape on err.message, since escape is being used in cdata already?

Removing the CDATA tag is not a change that would go into this major version, for the record. Unless it's really pointless.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you are removing the CDATA tag because if you are escaping the inner content then there's apparently no point for the CDATA tag (?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dasilvacontin
I am removing the CDATA tag because if you are escaping the inner content then there's no point for the CDATA tag

Copy link
Contributor

Choose a reason for hiding this comment

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

Gonna read a bit more on the CDATA tag...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dasilvacontin pinging you in https://gitter.im/graingert

} else if (test.isPending()) {
this.write(tag('testcase', attrs, false, tag('skipped', {}, true)));
} else {
Expand Down Expand Up @@ -164,11 +164,3 @@ function tag(name, attrs, close, content) {
}
return tag;
}

/**
* Return cdata escaped CDATA `str`.
*/

function cdata(str) {
return '<![CDATA[' + escape(str) + ']]>';
}