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

Janky cli test output #7

Open
lloydk opened this issue Nov 28, 2023 · 20 comments
Open

Janky cli test output #7

lloydk opened this issue Nov 28, 2023 · 20 comments
Labels
bug Something isn't working
Milestone

Comments

@lloydk
Copy link
Contributor

lloydk commented Nov 28, 2023

When running the color.js cli tests on Ubuntu 20.04/WSL2 running in Windows Terminal 1.18.2822.0 the test output is almost unreadable.

image
image

Commenting out the call to logUpdate fixes the problem.

logUpdate(tree);

@LeaVerou
Copy link
Owner

Yeah, bugs like this are the primary reason it's not released yet. I need to either make the output more compact by default, or do something smart when it exceeds the terminal window height, otherwise everything just goes to 💩

@LeaVerou LeaVerou added the bug Something isn't working label Mar 16, 2024
@LeaVerou LeaVerou added this to the Launch milestone Mar 16, 2024
@DmitrySharabin
Copy link
Collaborator

@lloydk, could you please check whether you're still facing the issue? We pushed some changes that should fix it.

@lloydk
Copy link
Contributor Author

lloydk commented May 29, 2024

Running the color.js tests with this commit fixes the janky output but I encountered the following issues:

Expanding a node doesn't show all child nodes. For example pressing from the root node results in this output:
image

I would expect to see all children of the expanded node.

There's no visual indication of which node I'm on so navigating the tree is difficult.

It would be nice to have a quick way to see the failing tests. Right now it requires multiple keystrokes to see the failing test output.

@LeaVerou
Copy link
Owner

@lloydk Thanks for the great feedback! So, just to make sure I get this right, the interactive CLI does show all tests unless you use that commit, right?

There's no visual indication of which node I'm on so navigating the tree is difficult.

Ouch, we definitely need to fix that! There's supposed to be, see @DmitrySharabin’s video: #27 (comment)

What OS/Terminal app are you using?

It would be nice to have a quick way to see the failing tests. Right now it requires multiple keystrokes to see the failing test output.

You mean like a flat summary of all failing tests?

@lloydk
Copy link
Contributor Author

lloydk commented May 30, 2024

@lloydk Thanks for the great feedback! So, just to make sure I get this right, the interactive CLI does show all tests unless you use that commit, right?

Using the version that color.js is referencing still produces janky output and is non-interactive. The latest commit is interactive but doesn't show all child nodes (you can see that in the video you referenced below).

There's no visual indication of which node I'm on so navigating the tree is difficult.

Ouch, we definitely need to fix that! There's supposed to be, see @DmitrySharabin’s video: #27 (comment)

What OS/Terminal app are you using?

I'm using MS Terminal on Windows 10. If I set the Intense text style setting to "Bold font with bright colors" instead of "Bright colors" (which is the default) it works.

image

It would be nice to have a quick way to see the failing tests. Right now it requires multiple keystrokes to see the failing test output.

You mean like a flat summary of all failing tests?

Not sure what the best/easiest approach would be, I just know that having to navigate the tree expanding nodes to see the failing test output will get old pretty quickly.

A flat summary could be an option, automatically expanding the tree if there are nodes with failing tests is another possibility. Not sure how feasible the second option is.

@DmitrySharabin
Copy link
Collaborator

Actually, we can try to address all the mentioned issues and not use any of the third-party libraries for Terminal colors (until we have to).

Expanding a node doesn't show all child nodes.

This is the behavior we added to make the output a bit less noisy and not showing groups with all tests pass. However, if someone needs to see all the groups, there is no way to do it for now—even the verbose option doesn't help.

But this can be easily fixed by adding to this lines one more check:

ret.children = this.tests.filter(t => t.stats.fail + t.stats.pending + t.stats.skipped > 0)

- ret.children = this.tests.filter(t => t.stats.fail + t.stats.pending + t.stats.skipped > 0) 
+ ret.children = this.tests.filter(t => o?.verbose || t.stats.fail + t.stats.pending + t.stats.skipped > 0)

There's no visual indication of which node I'm on so navigating the tree is difficult.

To address this, I have three options to suggest (in the order from ”I don't like it, but it's an option” to ”That might work” 😃)

Option 1—underline

Not really helpful from my perspective.

SCR-20240530-jvqp

Option 2—reversed colors

More readable but a bit ugly (to my taste).

SCR-20240530-jwal

Option 3—colored background

I tried different colors from the palette we support (including their light shades):

let hues = ["black", "red", "green", "yellow", "blue", "magenta", "cyan", "white"];

But this one seems the most readable (you can see it in action in the videos below).

SCR-20240530-jvgy

A flat summary could be an option, automatically expanding the tree if there are nodes with failing tests is another possibility. Not sure how feasible the second option is.

We can expand the tree if there are nodes with failing tests. It might be helpful to specify which nodes should be expanded: either with failed tests, skipped tests, or both (as an experiment, I added an option for this).

By default, if the verbose mode is not activated, we'll get the following:

Expand.fail.mp4

If the verbose mode is activated, the expanded tree might mess up the output (if some groups have many tests, not only failed), so I would prefer not to automatically expand nodes with failed (skipped) tests. We still might get a messy output if there are many failed tests, but it's much better than nothing.

Consider.verbose.mp4

P.S. The corresponding fix is a couple of clicks away. 🙃

@LeaVerou
Copy link
Owner

The problem with the background is that it's very hard to guarantee adequate contrast with all the different terminal themes people use. E.g. even in those videos there are many areas where the text is hard to read. The bold was ideal, but it looks like it doesn't work everywhere? Another solution would be to change the symbol and color of the arrow itself, e.g. have it be ▷ when not selected and a green ▶ when selected, plus the bold wherever that works.

Also, I noticed we output all the console messages when the runner finishes. Can we print them as they happen or would that mess up the output?

@DmitrySharabin
Copy link
Collaborator

Another solution would be to change the symbol and color of the arrow itself, e.g. have it be ▷ when not selected and a green ▶ when selected, plus the bold wherever that works.

That's an excellent idea! I'll try this and post a video with the result here.

Can we print them as they happen or would that mess up the output?

It would. Actually, this is the exact reason why we intercept console and replay all the intercepted messages before the tree with the final results. See this thread of comments: #29 (comment)

@DmitrySharabin
Copy link
Collaborator

Here is how it might look:

Active.node.mp4

@lloydk
Copy link
Contributor Author

lloydk commented May 30, 2024

That looks good

@LeaVerou
Copy link
Owner

@lloydk The reason you don't see all child tests is because they’re passing, but we should definitely print something like "…and N passing tests" to make that clear.

It would. Actually, this is the exact reason why we intercept console and replay all the intercepted messages before the tree with the final results. See this thread of comments: #29 (comment)

My understanding is that the reason this produced janky output is that the console messages are printed whenever. What I was saying is, we intercept the console, but print out the messages that have been emitted so far before all the test output, but printing them on every re-render, rather than once at the end.

Here is how it might look:

That looks great!

@LeaVerou
Copy link
Owner

Wrt seeing all tests and verbose vs not verbose etc, how about this proposal:

  • Tree is all collapsed, so unless you expand it you only see one line
  • When you expand the root, every node gets expanded, but you only see failed tests
  • There is a collapsed node at the end "M passing tests, N skipped tests" that can be expanded to show them, no need for special flags.

Thoughts?

Alternatively (or in addition), we could support keyboard shortcuts for "Expand All" and "Collapse All" e.g. Shift + arrow.

@lloydk
Copy link
Contributor Author

lloydk commented May 30, 2024

@lloydk The reason you don't see all child tests is because they’re passing, but we should definitely print something like "…and N passing tests" to make that clear.

I had a feeling it was something like that but I think I was confused by the DeltaE tests which have zero failing tests but some skipped tests. I wouldn't lump skipped tests in with failing tests.

@lloydk
Copy link
Contributor Author

lloydk commented May 30, 2024

  • When you expand the root, every node gets expanded, but you only see failed tests

Every node or every node with failing tests? If it's the latter then I think your proposal might work.

@DmitrySharabin
Copy link
Collaborator

What I was saying is, we intercept the console, but print out the messages that have been emitted so far before all the test output, but printing them on every re-render, rather than once at the end.

Oh, now I see what you mean. That makes perfect sense. Not sure whether it'll work. Let me try this out and see what we can get.

Alternatively (or in addition), we could support keyboard shortcuts for "Expand All" and "Collapse All" e.g. Shift + arrow.

This will be an excellent addition, regardless. 👍

no need for special flags.

Seconded!

Every node or every node with failing tests? If it's the latter then I think your proposal might work.

If I'm not mistaken, the first part of the tree will contain only failed tests, so when we expand the root, all nodes (i.e., nodes with failed tests) will expand.

@LeaVerou
Copy link
Owner

LeaVerou commented May 30, 2024

  • When you expand the root, every node gets expanded, but you only see failed tests

Every node or every node with failing tests? If it's the latter then I think your proposal might work.

The latter, though do note the proposal to have a dummy collapsed node for the skipped and passed tests. So it could still get noisy.

@LeaVerou
Copy link
Owner

Actually, I just had an idea. So console messages aren't actually that useful in isolation, they are far more useful when associated with a specific test. What if tests themselves could also be expanded to get more info about them? E.g. tests with console messages could say e.g. "3 console messages", and expanding them would show the console messages. That also makes the non-significant ones easy to ignore AND makes the output a lot more tidy!

@DmitrySharabin
Copy link
Collaborator

I like this idea a lot!

I have a question. Do we expect it to become a part of a test's results? Or is this feature only for CLI output? When running tests in the browser, all messages will be printed out anyway.

@LeaVerou
Copy link
Owner

I like this idea a lot!

I have a question. Do we expect it to become a part of a test's results? Or is this feature only for CLI output? When running tests in the browser, all messages will be printed out anyway.

This is a separate piece of data. A test could have e.g. 2 passed, 3 failed, 1 skipped, and 2 console messages. Unlike the other stats which are recursive, console messages would only be shown under the narrowest tree they appear in (but the number of console messages should still be recursive).

We'll probably have to completely override console messages for this, i.e. print out the messages ourselves, but I think the UX gain is worth it.

When running tests in the browser, all messages will be printed out anyway.

That could go either way. It’s not necessary in the browser, but it can still be useful to know which tests caused which console log messages, or to suspend them until some kind of user action (I often have to isolate tests just to avoid getting a flood of console messages while debugging).

@DmitrySharabin
Copy link
Collaborator

This is a separate piece of data. A test could have e.g. 2 passed, 3 failed, 1 skipped, and 2 console messages. Unlike the other stats which are recursive, console messages would only be shown under the narrowest tree they appear in (but the number of console messages should still be recursive).

I see. Thanks!

We'll probably have to completely override console messages for this, i.e. print out the messages ourselves, but I think the UX gain is worth it.

Agreed. Need to experiment with this a bit to see what we might get ASAP.

That could go either way. It’s not necessary in the browser, but it can still be useful to know which tests caused which console log messages, or to suspend them until some kind of user action (I often have to isolate tests just to avoid getting a flood of console messages while debugging).

Makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants