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

Fix a bug where the NUnit-Report fails to generate if the test output contains Virtual Terminal Sequences #2511

Merged
merged 13 commits into from
Jul 1, 2024

Conversation

ocalvo
Copy link
Contributor

@ocalvo ocalvo commented Jun 25, 2024

PR Summary

The following test will fail to generate an NUnit report:

Describe 'Describe VT Sequences' {
  Write-Output "`e[32mHello World`e[0m"
  $true | Should -Be $true
}

This will fail because WriteCData does not support writting the ESC character, which is a valid VT sequence.
The fix is to replace the ESC and Bell characters for {ESC} and {BELL}.
According to the Xml CDATA spec, there is no way to escape these characters, other than failing, but we still need to represent them in the NUnit report.
Afte this change, these characters will be represented as by its printable version in the unicode table: for the ESC (0x1B) and as for the BELL (0x07) in the resulting Xml CData for example.

PR Checklist

  • PR has meaningful title
  • Summary describes changes
  • PR is ready to be merged
    • If not, use the arrow next to Create Pull Request to mark it as a draft. PR can be marked Ready for review when it's ready.
  • Tests are added/update (if required)
  • Documentation is updated/added (if required)

@ocalvo ocalvo marked this pull request as draft June 25, 2024 01:23
@nohwnd
Copy link
Member

nohwnd commented Jun 25, 2024

Hello, thanks for the PR, the test you added is failing. I assume you also want this backported to version 5.x.x ?

@ocalvo ocalvo marked this pull request as ready for review June 25, 2024 17:24
@ocalvo
Copy link
Contributor Author

ocalvo commented Jun 25, 2024

Hello, thanks for the PR, the test you added is failing. I assume you also want this backported to version 5.x.x ?

No, we can easily update our test suite to latest version.

@ocalvo
Copy link
Contributor Author

ocalvo commented Jun 25, 2024

@nohwnd I have fixed the test, it was failling because WindowsPowerShell does not escape the ESC character. Fixed using the ASCII value explicitly.

@ocalvo
Copy link
Contributor Author

ocalvo commented Jun 27, 2024

Hi @nohwnd , we are blocked by this, we would appreciate if you could review at your earliest convenience.

@nohwnd nohwnd merged commit 718ba6d into pester:main Jul 1, 2024
11 checks passed
@nohwnd
Copy link
Member

nohwnd commented Jul 1, 2024

/backport rel/5.6.x

@nohwnd
Copy link
Member

nohwnd commented Jul 1, 2024

/backport to rel/5.6.x

Copy link

github-actions bot commented Jul 1, 2024

Started backporting to rel/5.6.x: https://github.com/pester/Pester/actions/runs/9744568632

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