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

Add line wrap disablement to ConsoleReporter #433

Merged
merged 2 commits into from
Nov 24, 2024

Conversation

edmundnoble
Copy link
Contributor

@edmundnoble edmundnoble commented Nov 13, 2024

I don't propose changing cabal.project, this is just for a demo until the PR to ansi-terminal is merged (UnkindPartition/ansi-terminal#171)

I saw that our test suite leaves garbage in the terminal output despite using --hide-successes. This turns out to be worse the narrower my terminal is. The more general problem is that ConsoleReporter leaves garbage in the terminal when the lines it prints are wrapped, because it doesn't know to clear multiple lines, just the most recent one. This PR fixes that by disabling line wrapping while printing that line.

This PR makes ConsoleReporter's ANSI tricks clean up after themselves properly in the presence of a narrow terminal, a long test name, or a long progress message, including those printed by testCaseWithSteps from tasty-hunit. It also prevents test group names from wrapping, because that has the same cause.

@edmundnoble edmundnoble force-pushed the push-xyyustvzkmsq branch 3 times, most recently from 9240df9 to ba15ca8 Compare November 13, 2024 05:03
Copy link
Collaborator

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

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

That's a nice idea! Let's wait for UnkindPartition/ansi-terminal#171 to be released, then I'll prepare a more in-depth review.

-- line will be cleared later or the test tree printing will
-- itself wrap.
withoutLineWrap :: IO () -> IO ()
withoutLineWrap m = do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we use bracket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know really what the best practice is here. Printing is quick so I don't see it being interrupted by an async exception. Printing could result in an exception caused by stdout becoming unavailable, but in that case we can't print the enableLineWrap code anyway, so there is no recovery.

It does seem natural to use bracket though. It's up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did it for now.

@Bodigrim
Copy link
Collaborator

Please update the Cabal file with ansi-terminal >= 1.1.2.

@Bodigrim
Copy link
Collaborator

Or, even better, let's guard usage of disableLineWrap / enableLineWrap by #if MIN_VERSION_ansi_terminal(1,1,2) and use no-op instead otherwise.

@edmundnoble
Copy link
Contributor Author

edmundnoble commented Nov 15, 2024

@Bodigrim ansi-terminal 1.1.2 has been released and I've updated this PR to have it as a minimum bound. 🥳

Edit: I didn't see your above comments, github issue.

@edmundnoble edmundnoble force-pushed the push-xyyustvzkmsq branch 2 times, most recently from bec56cf to ecd50f0 Compare November 15, 2024 21:02
@edmundnoble
Copy link
Contributor Author

The latest update should address all feedback thus far.

Copy link
Collaborator

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

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

We certainly benefit from no line wrapping when executing tests with --hide-successes. But do we benefit from disabling line wrapping in the absence of --hide-successes? Maybe it should be used only when --hide-successes is enabled?..

core/Test/Tasty/Ingredients/ConsoleReporter.hs Outdated Show resolved Hide resolved
@edmundnoble
Copy link
Contributor Author

edmundnoble commented Nov 16, 2024

We certainly benefit from no line wrapping when executing tests with --hide-successes. But do we benefit from disabling line wrapping in the absence of --hide-successes? Maybe it should be used only when --hide-successes is enabled?..

I argue that we do benefit from disabling line wrapping without --hide-successes: progress indicators get cleared even without --hide-successes, so this bug remains, especially using testCaseWithSteps. Also when printing test group names, we keep indenting further to the right, and on a narrow terminal, this may cause wrapping for longer test and group names.

@Bodigrim
Copy link
Collaborator

I argue that we do benefit from disabling line wrapping without --hide-successes: progress indicators get cleared even without --hide-successes, so this bug remains, especially using testCaseWithSteps.

That's a good argument.

My main concern is that without line wrapping resultShortDescription will be invisible on a narrow terminal. Which is especially undesirable if the result is FAIL. I think we should switch line wrapping on before printing resultShortDescription always or at least in case it is FAIL.

@edmundnoble
Copy link
Contributor Author

I argue that we do benefit from disabling line wrapping without --hide-successes: progress indicators get cleared even without --hide-successes, so this bug remains, especially using testCaseWithSteps.

That's a good argument.

My main concern is that without line wrapping resultShortDescription will be invisible on a narrow terminal. Which is especially undesirable if the result is FAIL. I think we should switch line wrapping on before printing resultShortDescription always or at least in case it is FAIL.

That's an excellent point. I undid that part of the change, now the result is not wrapped.

@edmundnoble edmundnoble marked this pull request as ready for review November 21, 2024 01:57
@Bodigrim Bodigrim merged commit d7efcd4 into UnkindPartition:master Nov 24, 2024
14 checks passed
@Bodigrim
Copy link
Collaborator

Thanks, that's awesome!

@edmundnoble edmundnoble deleted the push-xyyustvzkmsq branch November 26, 2024 22:06
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