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

Only un-escape unicode character encoding color codes Fix #1124 #1147

Merged
merged 3 commits into from
Sep 4, 2024

Conversation

Socolin
Copy link
Contributor

@Socolin Socolin commented Dec 17, 2023

Fixes #1124

@Socolin
Copy link
Contributor Author

Socolin commented Dec 17, 2023

I was not able to run the tests on my computer, but they seems to work in github action.

@OsirisTerje
Copy link
Member

@Socolin This is still open, build issues, merge conflicts. Any chance of having a look?

@Socolin
Copy link
Contributor Author

Socolin commented May 13, 2024

I'll take a look I did not saw the build error I was waiting for some feedback on this.

@OsirisTerje
Copy link
Member

Your unit test is failing. See Details, nearly near the bottom.

@Socolin
Copy link
Contributor Author

Socolin commented Jul 18, 2024

Hello, the test result is not available anymore could you trigger the pipeline again so I can see the errors ? I will have time again to look at this in the coming weeks.

@OsirisTerje
Copy link
Member

OsirisTerje commented Jul 18, 2024

I did a manual merge from main (who had changed) on the web here, and got in some compiler errors. I think it is faster if you just fix those than for me to clone your repo. Then the build should work again. It seems to only be some small glitches, braces etc

@Socolin
Copy link
Contributor Author

Socolin commented Jul 27, 2024

Sorry for the delay, I finally got the time to take a look, I rebased the branch + force push and did the fix, all should be good now :)

@OsirisTerje
Copy link
Member

Lgtm, any comments @manfred-brands , @stevenaw ?

Copy link
Member

@stevenaw stevenaw left a comment

Choose a reason for hiding this comment

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

This looks great thanks for your contribution @Socolin

No concerns from what I can research about this, but I admit that console colors are not something I know much about. I'd appreciate another pair of eyes from a team member. Would it make sense to add a few more negative tests for cases with leading [ but otherwise invalid formats after?

[TestCase("\\u001", "\\u001")]
[TestCase("\\u01", "\\u01")]
[TestCase("\\u1", "\\u1")]
[TestCase("\\u001b6", "\u001b6")]
[TestCase("\\u001b6", "\\u001b6")]
[TestCase("\\u001b[0m", "\u001b[0m")]
Copy link
Member

Choose a reason for hiding this comment

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

request: Would it make sense to add a test for a few more invalid inputs, such as a leading [ but missing trailing m ?

Copy link
Member

@stevenaw stevenaw left a comment

Choose a reason for hiding this comment

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

Actually, I'm going to go ahead and approve as I don't want to necessarily hold up merging over this, but I'd appreciate understanding if we thought it a case we'd want a test over

@OsirisTerje OsirisTerje added this to the 4.7 milestone Sep 4, 2024
@OsirisTerje OsirisTerje merged commit 13f8d23 into nunit:master Sep 4, 2024
3 checks passed
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.

Mangled test output regression after problematic fix
3 participants