Skip to content

Conversation

@vlada-shubina
Copy link
Member

fixes #8958

Context

TerminalLogger in .NET 8.0.100-preview.6 issues audible alerts on iTerm2
Iterm2 treats ;9 code differently from ConEmu: https://iterm2.com/documentation-escape-codes.html

Changes Made

Disabled progress reporting on Mac

Testing

Tested manually on Mac

Notes

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Having to split every validation test output in 3 is a bit unfortunate but this works for me.

. . . Hmm, actually, can we set the mock terminal in the tests to not emit this code consistently except for a specific test for this behavior? Up to you whether that seems worth it.

@vlada-shubina
Copy link
Member Author

Having to split every validation test output in 3 is a bit unfortunate but this works for me.

. . . Hmm, actually, can we set the mock terminal in the tests to not emit this code consistently except for a specific test for this behavior? Up to you whether that seems worth it.

It depends on test intention actually. If the intention is to test real life e2e scenario, and Mac output is currently indeed different, it's better to keep them split as is.

Unless we "unseal" Terminal implementation, and override only diff members, or make it configurable, the mock implementation can easily deviate and bug will be missed. Given that most of us is developing on Win, Mac and Linux issues can be unnoticed.

From my experience, it's worth to keep truly e2e scenarios unmocked. For specific tests, for example DisplayNodesShowsCurrent, DisplayNodesOverwritesWithNewTargetFramework it might be reasonable to use mock, however i don't have a full picture to make a decision for them.

@vlada-shubina vlada-shubina added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jul 26, 2023
@vlada-shubina
Copy link
Member Author

Unless there is more feedback on open items, this PR is ready to be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TerminalLogger in .NET 8.0.100-preview.6 issues audible alerts on iTerm2

5 participants