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

avoid double and tripple xUnit XML escaping #2178

Merged

Conversation

graingert
Copy link
Contributor

No description provided.

@graingert
Copy link
Contributor Author

Otherwise the title gets triple escaped and the error gets double escaped

<testcase classname="ex-name" name="should do a thing" time="0.12"><failure><![CDATA[(a =&amp;gt; b + &amp;quot;c&amp;quot;); failed
Error: (a =&gt; b + &quot;c&quot;);
    at Object.&lt;anonymous&gt; (./foo.js:80:19)]]></failure></testcase>
</testsuite>

@graingert
Copy link
Contributor Author

@danielstjules bump

@graingert
Copy link
Contributor Author

@dasilvacontin bump

*/

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

Choose a reason for hiding this comment

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

Where did the CDATA tag go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@graingert graingert force-pushed the prevent-double-tripple-escaping branch from d053a37 to c7074c4 Compare April 13, 2016 10:24
@dasilvacontin
Copy link
Contributor

Okay, without the CDATA tag, rendering is identical. node content is equivalent but not the same, though apparently no one cares about that. I'm waiting for @jkrall's reply, just in case: https://github.com/mochajs/mocha/pull/179/files#r59530748.

Otherwise LGTM. But hopefully we can get a reply soon.

@dasilvacontin
Copy link
Contributor

dasilvacontin commented May 23, 2016

Will locally rebase && test in a few hours and merge if all is good.

@dasilvacontin
Copy link
Contributor

Sauce passed when ran from local. Merging. Thanks @graingert! :)

@dasilvacontin dasilvacontin merged commit 49b5ff1 into mochajs:master May 23, 2016
@graingert
Copy link
Contributor Author

Sweet do you know when this will get a release?

@dasilvacontin
Copy link
Contributor

@graingert Now available on npm. ;) (v2.5.2)

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.

2 participants