Use ANSI colors for test results; more polish #771

bioball · 2024-11-01T22:08:35Z

Any thrown Pkl Errors are colored in the simple test report!

Also:

Refactor TextFormatter to be more generic; rename to TextFormattingStringBuilder
Adjust test report slightly (no emojis, add more spacing).
Introduce ColorTheme class.
Make stack frame descriptors colored as "faint"

Also: this changes the summary so it summarizes all modules, rather than a summary per module.

Note: I borrowed some of this styling from mocha; I've always liked how they style their test results.

Screenshots:

FYI @jjmaestro

jjmaestro · 2024-11-01T23:32:40Z

Maybe the bodies of Expected and Actual in the example error could be in the "highlighted red" that's used in fact, I think it reads better :-?

Are the colors user-configurable? (I'm on mobile so I haven't been able to check the code changes yet).

bioball · 2024-11-01T23:45:18Z

Are the colors user-configurable? (I'm on mobile so I haven't been able to check the code changes yet).

No, but we can add that in the future; that requires some design work that we don't quite have time for (we're trying to get this in before 0.27 ships on Monday).

bioball · 2024-11-01T23:50:00Z

pkl-core/src/main/java/org/pkl/core/runtime/TextFormattingStringBuilder.java

+  }
+
+  public TextFormattingStringBuilder append(String value) {
+    appendCodes();


Optimization: only append the delta that's needed.

new TextFormattingStringBuilder(true) .append(AnsiColor.RED, "Hi") .append(AnsiColor.RED, "hi") .append(EnumSet.of(AnsiColor.RED, AnsiColor.BOLD), "hi")) .toString()

Results in:

\033[31m;Hihi\033[1mhi[\0m

bioball · 2024-11-02T06:15:14Z

Good point re: adding better colors to the failure message.

Here's an updated screenshot!

Also: * Refactor `TextFormatter` to be more generic (less indirection). * Adjust test report slightly (no emojis, add more spacing).

jjmaestro · 2024-11-03T01:17:59Z

Good point re: adding better colors to the failure message.

Here's an updated screenshot!

🔥🎉👌

IMHO soooo much better!! Thanks!! 🙏

bioball · 2024-11-03T01:34:27Z

pkl-core/src/main/java/org/pkl/core/runtime/StackTraceRenderer.java

+                out.append("at ")
+                    .append(frame.getMemberName() != null ? frame.getMemberName() : "<unknown>")
+                    .append(" (")
+                    .appendUntrusted(frame.getModuleUri())


These values might get transformed by ~/.pkl/settings.pkl, might inject ANSI escape codes.

Protect against that by adding reset, and re-applying all ansi codes.

holzensp

At this point, the "formatter" does so little beyond StringBuilder; isn't it better to just use a normal StringBuilder, define some static final constants for the ANSI codes and put a if (!color) { output.replaceAll("\u001B\\[[;\\d]*m", ""); } at the point of StringBuilder::toString?

Approving to unblock for the release.

pkl-core/src/main/java/org/pkl/core/runtime/TestRunner.java

holzensp · 2024-11-03T21:11:08Z

pkl-core/src/main/java/org/pkl/core/runtime/TestRunner.java

+    sb.append(
+        ColorTheme.TEST_FAILURE_MESSAGE,
+        () -> {
+          appendLocation(sb, location);
+          sb.append("\n  Expected: ");
+          appendLocation(sb, expectedLocation);
+          sb.append("\n  ");
+          sb.append(ColorTheme.TEST_EXAMPLE_OUTPUT, expectedValue.replaceAll("\n", "\n  "));
+          sb.append("\n  Actual: ");
+          appendLocation(sb, actualLocation);
+          sb.append("\n  ");
+          sb.append(ColorTheme.TEST_EXAMPLE_OUTPUT, actualValue.replaceAll("\n", "\n  "));
+        });


Mixing different abstractions patterns; StringBuilder, state parentheses, functional string building... is the closure-encapsulation purely to protect against the optimization built into the string builder? If so, that seems a leaky abstraction.

Suggested change

sb.append(

ColorTheme.TEST_FAILURE_MESSAGE,

() -> {

appendLocation(sb, location);

sb.append("\n Expected: ");

appendLocation(sb, expectedLocation);

sb.append("\n ");

sb.append(ColorTheme.TEST_EXAMPLE_OUTPUT, expectedValue.replaceAll("\n", "\n "));

sb.append("\n Actual: ");

appendLocation(sb, actualLocation);

sb.append("\n ");

sb.append(ColorTheme.TEST_EXAMPLE_OUTPUT, actualValue.replaceAll("\n", "\n "));

});

sb

.append(ColorTheme.TEST_FAILURE_MESSAGE, "(")

.appendUntrusted(ColorTheme.TEST_FAILURE_MESSAGE, location)

.append(ColorTheme.TEST_FAILURE_MESSAGE, ")\n Expected: (")

.appendUntrusted(ColorTheme.TEST_FAILURE_MESSAGE, expectedLocation)

.append(ColorTheme.TEST_FAILURE_MESSAGE, ")\n ")

.append(ColorTheme.TEST_EXAMPLE_OUTPUT, expectedValue.replaceAll("\n", "\n "))

.append(ColorTheme.TEST_FAILURE_MESSAGE, "(")

.appendUntrusted(ColorTheme.TEST_FAILURE_MESSAGE, actualLocation)

.append(ColorTheme.TEST_FAILURE_MESSAGE, ")\n ")

.append(ColorTheme.TEST_EXAMPLE_OUTPUT, actualValue.replaceAll("\n", "\n "))

is the closure-encapsulation purely to protect against the optimization built into the string builder

No; the purpose of the runnable is so you can have a a block where:

You don't need to keep repeating the same style for each append.

The parent style applies to every element appended within, including styles applied on the children. For example:
sb.append(AnsiCode.RED, () -> { sb.append(AnsiCold.BOLD, "Hello"); });
This results in "hello" being both red and bold.

On not repeating; that also holds true for setting a style with a separate call, as in the previous approach.

Considering the other discussions where this AnsiCodingStringBuilder ties the user into using formulations of the ANSI model, why not just fully commit to that and avoid ad hoc primitives like this closure-nesting and appendUntrusted, that live neither in ANSI, nor in StringBuilder? That way, the user only needs to know those two models. What I'm suggesting is:

Provide a append(EnumSet<AnsiCode> codes) and/or append(AnsiCode code) that inserts escape codes.

Optionally provide a EnumSet<AnsiCode> getCurrentAnsiCodes() that helps locally revert to an old state.

Implement useColor by either making said append overloads conditional, or doing a regex replacement in toString to fish all escapes out.

This API, in its current form, lets us abstract and introduce another formatter later on (for example, HTML colors).

Your suggestion ties us much more strongly to ANSI (reset is a not a thing in HTML). It's also harder to use (users have to remember how reset works and how to apply previous colors).

var prevColors = sb.getCurrentAnsiCodes(); sb.append(AnsiCode.RED); sb.append("hello, "); for (var name : names) { sb.append(name).append(" "); } sb.reset(); sb.append(prevColors);

Compared to:

sb.append(AnsiCode.RED, () -> { sb.append("hello, "); for (var name : names) { sb.append(name).append(" "); } });

To build on my abstraction point: if we wanted to later have an HtmlCodingStringBuilder, this would look something like:

class HtmlCodingStringBuilder implements ColorCodingStringBuilder [ public ColorCodingStringBuilder append(Color color, Runnable runnable) { builder.append("<span style=\"color: ").append(getHex(color)).append("\">"); runnable.run(); builder.append("</span>"); } }

pkl-core/src/main/java/org/pkl/core/runtime/TextFormattingStringBuilder.java

holzensp · 2024-11-03T21:13:09Z

pkl-core/src/main/java/org/pkl/core/runtime/TextFormattingStringBuilder.java

+  public TextFormattingStringBuilder appendLine(int count) {
+    builder.append("\n".repeat(count));


pkl-core/src/main/java/org/pkl/core/util/ColorTheme.java

pkl-core/src/main/java/org/pkl/core/stdlib/test/report/SimpleReport.java

holzensp · 2024-11-03T22:34:42Z

pkl-core/src/main/java/org/pkl/core/runtime/TextFormattingStringBuilder.java

+  public enum Element {
+    PLAIN,
+    MARGIN,
+    HINT,
+    STACK_OVERFLOW_LOOP_COUNT,
+    LINE_NUMBER,
+    TEXT,
+    ERROR_HEADER,
+    ERROR,
+    RESET,
+    FAILING_TEST_MARK,
+    PASSING_TEST_MARK,
+    TEST_NAME,
+  }
+


holzensp · 2024-11-03T22:36:46Z

pkl-core/src/main/java/org/pkl/core/stdlib/test/report/JUnitReport.java

@@ -191,7 +197,11 @@ private VmTyped makeCdata(String text) {
    return new VmTyped(VmUtils.createEmptyMaterializedFrame(), clazz.getPrototype(), clazz, attrs);
  }

-  public static String renderXML(String indent, String version, VmDynamic value) {
+  private String stripColors(String str) {
+    return str.replaceAll("\u001B\\[[;\\d]*m", "");


Let's use either \033 or \u001b

Suggested change

return str.replaceAll("\u001B\\[[;\\d]*m", "");

return str.replaceAll("\u033\\[[;\\d]*m", "");

pkl-core/src/main/java/org/pkl/core/stdlib/test/report/SimpleReport.java

holzensp · 2024-11-04T15:08:00Z

pkl-core/src/main/java/org/pkl/core/stdlib/test/report/SimpleReport.java

+  private static final String passingMark = "✔ ";
+  private static final String failingMark = "✘ ";


Why the naming style deviation? If for readability (which I tend to agree with, because I'm alright with my tooling telling me something is static final), might we also convert the members of ColorTheme - which are not final - to camelCase?

holzensp · 2024-11-04T15:17:51Z

pkl-core/src/main/java/org/pkl/core/runtime/TestRunner.java

+    sb.append(
+        ColorTheme.TEST_FAILURE_MESSAGE,
+        () -> {
+          appendLocation(sb, location);
+          sb.append("\n  Expected: ");
+          appendLocation(sb, expectedLocation);
+          sb.append("\n  ");
+          sb.append(ColorTheme.TEST_EXAMPLE_OUTPUT, expectedValue.replaceAll("\n", "\n  "));
+          sb.append("\n  Actual: ");
+          appendLocation(sb, actualLocation);
+          sb.append("\n  ");
+          sb.append(ColorTheme.TEST_EXAMPLE_OUTPUT, actualValue.replaceAll("\n", "\n  "));
+        });


On not repeating; that also holds true for setting a style with a separate call, as in the previous approach.

Considering the other discussions where this AnsiCodingStringBuilder ties the user into using formulations of the ANSI model, why not just fully commit to that and avoid ad hoc primitives like this closure-nesting and appendUntrusted, that live neither in ANSI, nor in StringBuilder? That way, the user only needs to know those two models. What I'm suggesting is:

Provide a append(EnumSet<AnsiCode> codes) and/or append(AnsiCode code) that inserts escape codes.

Optionally provide a EnumSet<AnsiCode> getCurrentAnsiCodes() that helps locally revert to an old state.

Implement useColor by either making said append overloads conditional, or doing a regex replacement in toString to fish all escapes out.

holzensp · 2024-11-04T15:20:38Z

pkl-core/src/main/java/org/pkl/core/util/ColorTheme.java

+  public static AnsiCode ERROR_MESSAGE_HINT = AnsiCode.YELLOW;
+  public static AnsiCode ERROR_HEADER = AnsiCode.RED;
+  public static Set<AnsiCode> ERROR_MESSAGE = EnumSet.of(AnsiCode.RED, AnsiCode.BOLD);
+
+  public static AnsiCode STACK_FRAME = AnsiCode.FAINT;
+  public static AnsiCode STACK_TRACE_MARGIN = AnsiCode.YELLOW;
+  public static AnsiCode STACK_TRACE_LINE_NUMBER = AnsiCode.BLUE;
+  public static AnsiCode STACK_TRACE_LOOP_COUNT = AnsiCode.MAGENTA;
+  public static AnsiCode STACK_TRACE_CARET = AnsiCode.RED;
+
+  public static AnsiCode FAILING_TEST_MARK = AnsiCode.RED;
+  public static AnsiCode PASSING_TEST_MARK = AnsiCode.GREEN;
+  public static AnsiCode TEST_NAME = AnsiCode.FAINT;
+  public static AnsiCode TEST_FACT_SOURCE = AnsiCode.RED;
+  public static AnsiCode TEST_FAILURE_MESSAGE = AnsiCode.RED;
+  public static Set<AnsiCode> TEST_EXAMPLE_OUTPUT = EnumSet.of(AnsiCode.RED, AnsiCode.BOLD);


Considering none of these are final, I presume these are intended to become configurable, in which case, shouldn't they all be Set<AnsiCode>?

This isn't meant to be an API. I'll go ahead and make this final.

stackoverflow

Looks good. Left some comments. Nothing big.

pkl-core/src/main/java/org/pkl/core/runtime/TestRunner.java

stackoverflow · 2024-11-04T14:54:39Z

pkl-core/src/main/java/org/pkl/core/runtime/VmExceptionRenderer.java

@@ -39,44 +39,44 @@ public VmExceptionRenderer(@Nullable StackTraceRenderer stackTraceRenderer, bool

  @TruffleBoundary
  public String render(VmException exception) {
-    var formatter = TextFormatter.create(color);
+    var formatter = new AnsiCodingStringBuilder(color);


Suggested change

var formatter = new AnsiCodingStringBuilder(color);

var formatter = new AnsiCodingStringBuilder(hasColor);

I'd rename the attribute here. color gives the idea this is a color, not a boolean flag.

pkl-core/src/main/java/org/pkl/core/runtime/VmExceptionRenderer.java

pkl-core/src/main/java/org/pkl/core/runtime/AnsiCodingStringBuilder.java

Co-authored-by: Islon Scherer <[email protected]>

odenix · 2024-11-04T21:34:48Z

To make ANSI codes work reliably across platforms, you should:

call jansi's AnsiConsole.systemInstall() in cli.Main or commons.cli.cliMain
This might also fix Weird repl output and interactions #757.
leave automatic console detection to jansi
Right now, when I redirect stdout, I don't get colors for stderr.

Have you considered to use Jansi's formatter in pkl-core instead of rolling your own?
It's a tiny library that's already used in pkl-cli.
Alternatively, you could vendor the parts that you need in pkl-core (probably just the formatter).
In any case, pkl-core and pkl-cli should use the same formatter.

bioball · 2024-11-04T22:11:38Z

We did consider that.

IMO: I don't think Jansi adds too much value for formatting escape codes; writing those are pretty trivial and we don't need a library for that. However, the one valuable thing that Jansi provides is a better way to determine whether a stream is connected to a TTY or not; Java's System.console() is far too coarse. However, I think it's good enough for now, and we can improve it later by adding some JNI bindings (either ourselves or find a library for it).

AFAICT, Jansi doesn't have an API for "is stdout/stderr/stdin a tty?". If they did, I think we'd be happy to use it.

A follow-up step here is to get rid of Jansi as a dependency for formatting in pkl-cli and just use the formatter we've defined in pkl-core.

odenix · 2024-11-04T22:18:27Z

TL/DR: Replacing jansi's formatter is possible, but you'll need jansi for cross-platform console support in any case.

To solve the issues I listed, you'll either need to use Jansi or implement your own replacement.
But what's the point? Jansi is a tiny library, and the chance that you'll get cross-platform console support right is basically zero (just take a look at jansi's code).

As long as pkl-cli uses jansi, you should definitely call AnsiConsole.systemInstall() in cliMain, which will solve some Windows problems. (EDIT: will need some tweaks to work in native executable.) It will also fix automatic console detection for all platforms if you change your AUTO mode to behave the same as your ALWAYS mode.

PS: jline, which is used in pkl-cli, also depends on jansi.

bioball force-pushed the colored-test-reports branch 2 times, most recently from ac7c0a5 to f1270f1 Compare November 1, 2024 22:24

bioball commented Nov 1, 2024

View reviewed changes

Use ANSI colors for test results

b349bc4

Also: * Refactor `TextFormatter` to be more generic (less indirection). * Adjust test report slightly (no emojis, add more spacing).

bioball force-pushed the colored-test-reports branch from 697c11e to b349bc4 Compare November 2, 2024 07:02

bioball changed the title ~~Use ANSI colors for test results~~ Use ANSI colors for test results; more polish Nov 2, 2024

bioball commented Nov 3, 2024

View reviewed changes

holzensp reviewed Nov 3, 2024

View reviewed changes

bioball added 2 commits November 3, 2024 22:01

Address review

b2747db

Propagate colors

c689907

holzensp reviewed Nov 4, 2024

View reviewed changes

stackoverflow approved these changes Nov 4, 2024

View reviewed changes

holzensp approved these changes Nov 4, 2024

View reviewed changes

bioball and others added 3 commits November 4, 2024 13:25

Apply suggestions from code review

aa5c715

Co-authored-by: Islon Scherer <[email protected]>

Address review

87fd939

Run spotless apply

9ad4ec7

bioball merged commit 40a08af into apple:main Nov 4, 2024
5 checks passed

bioball deleted the colored-test-reports branch November 4, 2024 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ANSI colors for test results; more polish #771

Use ANSI colors for test results; more polish #771

bioball commented Nov 1, 2024 •

edited

Loading

jjmaestro commented Nov 1, 2024

bioball commented Nov 1, 2024

bioball Nov 1, 2024 •

edited

Loading

bioball commented Nov 2, 2024 •

edited

Loading

jjmaestro commented Nov 3, 2024 •

edited

Loading

bioball Nov 3, 2024

holzensp left a comment

holzensp Nov 3, 2024

bioball Nov 4, 2024 •

edited

Loading

holzensp Nov 4, 2024

bioball Nov 4, 2024 •

edited

Loading

holzensp Nov 3, 2024

holzensp Nov 3, 2024

holzensp Nov 3, 2024

holzensp Nov 4, 2024

holzensp Nov 4, 2024

holzensp Nov 4, 2024

bioball Nov 4, 2024

stackoverflow left a comment

stackoverflow Nov 4, 2024

odenix commented Nov 4, 2024 •

edited

Loading

bioball commented Nov 4, 2024 •

edited

Loading

odenix commented Nov 4, 2024 •

edited

Loading

		public TextFormattingStringBuilder appendLine(int count) {
		builder.append("\n".repeat(count));

	return str.replaceAll("\u001B\\[[;\\d]*m", "");
	return str.replaceAll("\u033\\[[;\\d]*m", "");

		private static final String passingMark = "✔ ";
		private static final String failingMark = "✘ ";

	var formatter = new AnsiCodingStringBuilder(color);
	var formatter = new AnsiCodingStringBuilder(hasColor);

Use ANSI colors for test results; more polish #771

Use ANSI colors for test results; more polish #771

Conversation

bioball commented Nov 1, 2024 • edited Loading

jjmaestro commented Nov 1, 2024

bioball commented Nov 1, 2024

bioball Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

bioball commented Nov 2, 2024 • edited Loading

jjmaestro commented Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

holzensp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bioball Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bioball Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackoverflow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odenix commented Nov 4, 2024 • edited Loading

bioball commented Nov 4, 2024 • edited Loading

odenix commented Nov 4, 2024 • edited Loading

bioball commented Nov 1, 2024 •

edited

Loading

bioball Nov 1, 2024 •

edited

Loading

bioball commented Nov 2, 2024 •

edited

Loading

jjmaestro commented Nov 3, 2024 •

edited

Loading

bioball Nov 4, 2024 •

edited

Loading

bioball Nov 4, 2024 •

edited

Loading

odenix commented Nov 4, 2024 •

edited

Loading

bioball commented Nov 4, 2024 •

edited

Loading

odenix commented Nov 4, 2024 •

edited

Loading