-
Notifications
You must be signed in to change notification settings - Fork 3
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
Comments
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 💩 |
@lloydk, could you please check whether you're still facing the issue? We pushed some changes that should fix it. |
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 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. |
@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?
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?
You mean like a flat summary of all failing tests? |
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).
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.
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. |
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).
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 But this can be easily fixed by adding to this lines one more check: https://github.com/LeaVerou/htest/blob/992a18ebaa1486a17695801eb280a60c58f3869f/src/classes/TestResult.js#L368 - 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)
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—underlineNot really helpful from my perspective. ![]() Option 2—reversed colorsMore readable but a bit ugly (to my taste). ![]() Option 3—colored backgroundI tried different colors from the palette we support (including their light shades): https://github.com/LeaVerou/htest/blob/992a18ebaa1486a17695801eb280a60c58f3869f/src/format-console.js#L12 But this one seems the most readable (you can see it in action in the videos below). ![]()
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.mp4If 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.mp4P.S. The corresponding fix is a couple of clicks away. 🙃 |
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? |
That's an excellent idea! I'll try this and post a video with the result here.
It would. Actually, this is the exact reason why we intercept |
Here is how it might look: Active.node.mp4 |
That looks good |
@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.
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.
That looks great! |
Wrt seeing all tests and verbose vs not verbose etc, how about this proposal:
Thoughts? Alternatively (or in addition), we could support keyboard shortcuts for "Expand All" and "Collapse All" e.g. Shift + arrow. |
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. |
Every node or every node with failing tests? If it's the latter then I think your proposal might work. |
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.
This will be an excellent addition, regardless. 👍
Seconded!
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. |
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. |
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! |
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.
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). |
I see. Thanks!
Agreed. Need to experiment with this a bit to see what we might get ASAP.
Makes sense. |
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.
Commenting out the call to
logUpdate
fixes the problem.https://github.com/LeaVerou/htest/blob/807def2987af3e864e9580fe6c06859c70c35f28/src/js/node-run.js#L65
The text was updated successfully, but these errors were encountered: