mtest: fix test output issues (in console) #9232
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9232 +/- ##
==========================================
+ Coverage 66.85% 69.94% +3.09%
==========================================
Files 386 193 -193
Lines 85058 42565 -42493
Branches 17551 8784 -8767
==========================================
- Hits 56862 29774 -27088
+ Misses 23402 10352 -13050
+ Partials 4794 2439 -2355
Continue to review full report at Codecov.
|
|
This pull request introduces 1 alert when merging 8726858 into 0fcd696 - view on LGTM.com new alerts:
|
|
Looks nice. Has this been tested on Windows? Its terminal is wonky. |
Not at all. Unfortunately I dont have access to a Windows machine (well I suppose I could set up a virtual machine of some sort) so that is definitely something I could use some help on testing. |
|
I see two issues trying this out:
on master that is
whereas on master it ends up with roughly one line per finished test, and the summary.
PR: |
trhd
force-pushed
the
tyrkky
branch
2 times, most recently
from
1fcaad0 to
e9b470e
Compare
|
Thanks @anarazel for pointing these out. I hopefully managed to fix these:
I have yet to try this out on Windows but...soon...hopefully. |
|
I managed to try this out in a virtual machine following mesonbuild.com's instructions for absolute beginners (needed help with Windows and Visual Studio). The performance of my virtual machine was terrible so I can't really say anything about timings, flickerings or such but everything looked OK to me. Didn't notice any issues with Windows' terminal. |
This change set aims to fix various "issues" seen with the current implementation. The changes can be summarized with the following list: * Replace emojis and spinners with multiline status displaying the name and running time of each currently running test. * The test output (especially in verbose mode or when multiple failing tests' output gets printed out) can get confusing. Try to make the output easier to read and grasp. Most notable change here is the addition of the test number to the beginning of each printed line. * Print exit details (i.e. exit code) of the test in verbose mode. * Try to make the verbose "live" output from tests to match the look and feel of otherwise produced (verbose) test output.
|
@anarazel did the change fix things for you? |
|
Has anybody looked at how Gitlab/GitHub/Travis deal with the multiline output and ESC[A sequences? This was the main reason why I went for the spinner. Also it would have been much better to split this in multiple commits. The changes to the logging of completed tests are completely independent from the ones to the progress. I will test it tomorrow on QEMU and report how it feels for TAP tests and in verbose mode. |
|
I quickly tried this out in Sourcehut and sadly it was as @bonzini suspected and the up/down cursor movement was not handled properly. If I had to guess this is due to the fact that we have TERM=xterm and insufficient ANSI escape character support. It seems that at least in Sourcehut the size of the TTY was reported as being 0x0 so we could perhaps do something like this which seemed to at least work in Sourcehut: But now I really have to go to bed... |
|
I tried it now and I see some problems, in that these changes completely undo some of the design points for the improved
Furthermore, these changes are not documented in the release notes. Also, the exit code was printed already before, see I would rather see this pull request reverted and submitted again as a separate set of changes that can be tested and evaluated individually. See #7830 for how to do it. |
|
I just got the windows portion for my postgres-built-via-meson project actually pass tests, and that showed that the new output seems problematic for windows. In a normal terminal the ansi control sequences seem to get broken for both the old and the new output at some point (after initially working), but the consequences with the new output are way worse. The not-deleted multi-line output ends up being very noisy: And it goes on like that for the rest of the test. |
|
@anarazel What is your "normal terminal"? Something different from what you end up with following this guide for windows? And this does't work with these changes now reverted either? @bonzini Thanks for the input. I now have fixes for the problems you mentioned but can currently test the Travis/GitLab/GitHub aspect only in Sourcehut. Maybe I'll squash those fixen in for a new PR? |
|
I can help with testing, just make small patches and fix one thing at a time. In particular for now I'd keep the spinner as is, because it also has the scrolling problem with very large parallelism. |
|
@anarazel the "normal" terminal (command prompt) is more or less broken, though the effect on the old (0.59) code should be limited to no emoji. The Windows "Terminal Preview" should work fine. I can help testing that using WSL. |
|
I believe all other known issues got fixed already (the last fixes didn't get merged) except:
The changes depend on each other quite heavily and that's why the commit became so large in the first place but I guess the spinner change is the one thing that could be splitted to a separate commit. I'll see what I can do. |
|
There are a lot of changes that can be split, such as:
and so on. Each of the above for example should be a separate commit; it's more work but it's really needed to do a meaningful review (and it would have allowed a more fine-grained revert). You don't need to create many PRs, just split changes into logical parts. |
Right, I noted that I saw a bit borked output on the "old" code as well. But that the damage is much more limited. FWIW, it didn't even work on the preview, until I change the system local to UTF-8, which seems to not work in other places :/. |
|
I think there's quite a bit of benefit in logging in a way where the "worst case" output on a broken terminal isn't too bad. |
I was annoyed by e.g. the meson's test subcommand's output's usage of the scissors emoji that never displayed properly in my development environments and set out to improve the look and feel of the test subcommand's output.
In the commit message I write:
which quite well describes the changes I believe (hope). A "picture" is however worth a thousand words:
https://asciinema.org/a/dGOULiN7z9t0OJXrAno570W7T
In my opinion the end result is better that the original. This is however quite a subjective matter and you might disagree...
In the case that people agree with me on the above I still feel that it would be good if other people tried this out as well before merging. AFAIK the "test output" is not really tested via meson's own tests and as I can check only so many corner cases on my own there are bound to be issues that got overlooked. Especially if someone has strong opinions on how the subtests should be reported I'd love to hear back from you.