Prevent Test From Hanging With AnimatedLabel Thread

[Kapral67]

The fix for #595 as proposed by #596 looks good to me.

The test proposed in #596 would hang with the AnimatedLabel Thread.

This PR proposes a modification so that now matter if Issue595 test fails or succeeds:

• it never hangs

• indicates test failure via:

  • output to stderr

  • returned exit code

[Kapral67]

redundant, can just call e.printStackTrace()

[avl42]

Things like e.printStackTrace() are often barfed at by code quality tools, like e.g. SonarQube.
The preferred way would be resorting to some logging-framework, so that it can be disabled
for production use - and not dump an exception-stackdump into user's face.

I'm aware that using a logging framework is not a simple decision, but the original code is already closer to a real solution than "simplifying" it back to e.printStackTrace().

My opinion: just leave that bit as it is. Simplifying it as suggested would be a step in the wrong direction.

[Kapral67]

Unless we resort to a logging framework

StringWriter s = new StringWriter();
e.printStackTrace(new PrintWriter(s));
System.err.print(s);

Is equivalent to e.printStackTrace() (at least in vanilla JDK implementations)

This code is also part of the test suite, so users will not see the trace produced by potential exceptions here

[mabe02]

This was copy+pasted from the bug report, I think :)

[mabe02]

Nevermind, wrong PR

[avl42]

Yes, they are functionally equivalent. And, if ever a logging framework is added, it would be changed to passing the exception to the logger directly, and not stringifying beforehand. (That was originally a thinko on my side, that the logger would then still need the stringified exception.) In the meantime, anyone could replace the "System.err" part by some other channel opened to some logfile, if he doesn't want exception stack-traces to go in user's face. (if terminal is in raw mode, those exceptions often appear ugly garbled, as the "\n" then is not mapped to a linebreak on terminal. If we changed it to the compact form, replacing the channel becomes more tedious. I don't see enough reason to compactify the code towards less flexibility. Afterall the two variants under discussion are equivalent at runtime.

…

On Thu, Feb 29, 2024 at 9:06 AM Maxwell Kapral ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/test/java/com/googlecode/lanterna/issue/Issue595.java <#597 (comment)>: > + return; + } + final Thread thread = optionalAnimatedLabelThread.get(); + thread.join(SLEEP_MILLIS); + if (thread.isAlive()) { + throw new IllegalStateException("AnimatedLabel Thread Waiting"); + } + + } catch (final Throwable e) { + if (e instanceof InterruptedException) { + Thread.currentThread() + .interrupt(); + } + ++exit_code; + final StringWriter stringWriter = new StringWriter(); + e.printStackTrace(new PrintWriter(stringWriter)); Unless we resort to a logging framework StringWriter s = new StringWriter();e.printStackTrace(new PrintWriter(s));System.err.print(s); Is equivalent to e.printStackTrace() (at least in vanilla JDK implementations) <https://github.com/openjdk/jdk/blob/998d0baab0fd051c38d9fd6021628eb863b80554/src/java.base/share/classes/java/lang/Throwable.java#L677-L679> — Reply to this email directly, view it on GitHub <#597 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIDBMWODDQD6AX4BQCTF3DYV3QRPAVCNFSM6AAAAABDRVJOZ6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMBYGIYTGMBQGQ> . You are receiving this because you commented.Message ID: ***@***.***>

[mabe02]

Given that this is a test for reproducing a reported bug, I don't really mind that the code style and some constructs are misaligned with the rest of the library. The code won't be included in the library anyway. I've intentionally avoided introducing or using any logging framework in the library itself since there never really was any strong need for logging anywhere. Rather, errors are thrown out and users should catch exceptions and log them as per their application.