mocha icon indicating copy to clipboard operation
mocha copied to clipboard

Published 20 hours ago • verified publisher icon mochajs

fix: remove ansi escape sequences from escaped xunit output

Open trieloff opened this issue 5 years ago • 12 comments

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

The utils.escape function is amended to strip the escape sequence using a regular expression.

Alternate Designs

  • patch the underlying he library: dismissed because the direct fix is likely faster to become usable
  • filter the output before running he: dismissed because the correct escaping in JavaScript is harder to find
  • filter out all invalid XML characters: dismissed because this is not a problem observed with other characters

Why should this be in core?

The xunit reporter is in core and should not be buggy

Benefits

xunit reporter output can be consumed by tools that expect valid XML

Possible Drawbacks

Tools that process the XML as plain (unicode) text (unlikely) can no longer see the ANSI escape sequence.

Applicable issues

fixes #4526

This is a bug fix (patch release)

trieloff avatar Dec 02 '20 18:12 trieloff

Coverage Status

Coverage remained the same at 94.143% when pulling d41753887fcba8aba06409a5ba2ab5509305bdaf on trieloff:issue/4526 into bc8ce0535d2b94c6f4cd3c23a0d9ccce87d6e53b on mochajs:master.

coveralls avatar Dec 02 '20 19:12 coveralls

There is a related xunit issue #3586.

juergba avatar Mar 09 '21 16:03 juergba

This PR hasn't had any recent activity, and I'm labeling it stale. Remove the label or comment or this PR will be closed in 14 days. Thanks for contributing to Mocha!

github-actions[bot] avatar Jul 09 '21 00:07 github-actions[bot]

This issue has been hanging around for 8 months now, and I'm unsure what I can do to get it moving again. There is an open thread that has multiple possible solutions to the problem, but no expression of preference from the Mocha team.

trieloff avatar Jul 19 '21 09:07 trieloff

👋 coming back to this @trieloff, are you still interested in working on this PR? As of https://github.com/mochajs/mocha/issues/5027 there are again maintainers who can review it now. No worries if you no longer have time - but if you do that'd be great!

JoshuaKGoldberg avatar Dec 02 '23 21:12 JoshuaKGoldberg

Hi @JoshuaKGoldberg – that's unexpected – but not unwelcome. I'm happy to rebase the PR to the latest main. Do you want to reopen this PR or have me create a new one?

trieloff avatar Dec 08 '23 12:12 trieloff

Great! Let's go ahead and reopen this one.

We also discussed that we don't like the requirement of rebasing, so if you want to do a more casual merge that works too. Whatever's easiest on your end. 🙂

JoshuaKGoldberg avatar Dec 08 '23 17:12 JoshuaKGoldberg

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: trieloff / name: Lars Trieloff (717474ab9616679ccc6039d4fd7d90a336d7393a, 9f3f8db00cac3db3c014a0bfeabf275e54728bc5, bfb62dc148d5f6e8be4d5171d34ce82a8c5dfb63, d41753887fcba8aba06409a5ba2ab5509305bdaf, b4b7392504a9f773833fe79bcec3261eb9bc4e32, 51cd81ca131ef6538411b4d6ace9e010979ed10c, 9d6dc63f4bfdfd11271ccf14e1f2c77371d947d9)

linux-foundation-easycla[bot] avatar Dec 08 '23 17:12 linux-foundation-easycla[bot]

Ok, let me get that CLA signed, then.

trieloff avatar Jan 03 '24 15:01 trieloff

  • CLA has been signed (thanks, corporate overlords)
  • master has been merged
  • conflicts have been resolved

trieloff avatar May 06 '24 12:05 trieloff

@JoshuaKGoldberg can I get some eyes on this, please?

trieloff avatar May 28 '24 09:05 trieloff

Yes! Thanks for the prod - this slipped off my radar. Reviewing now!

JoshuaKGoldberg avatar May 30 '24 20:05 JoshuaKGoldberg