-
Notifications
You must be signed in to change notification settings - Fork 43
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
Rely's output format doesn't look good when using dune runtest #240
Comments
Do you have an example repo I can clone and try it out? It wasn't immediately trivial for me to set up dune runtest in an example repo I had |
Sure! I've created a repo here: https://github.com/tmattio/dune-rely-demo
Then I've added two scripts:
|
The default Rely reporter is optimized for running directly in the terminal as opposed to with dune runtest (which captures stdout for diffing purposes and I imagine is getting tripped up by the ANSI escape sequences we use to give cleaner terminal output). If you want to use dune test stanzas and aren't using Pastel you could add In the long term, if you were interested in creating a reporter that was optimized for Dune Runtest that would be a welcome contribution. You can look at the TerminalReporter implementation in this repo or the CustomReporter API for the basics/I'd be happy to answer any questions you might have. |
Thanks @bandersongit! I'm loosing the color, but at least the output is readable! I might take a look at implementing a custom reporter yes, I'm trying to support releasing packages on Opam that use Rely, and using a test stanza is the only solution I've found. Would be cool to keep the colors as well 😄 |
Thanks for the repo, that was helpful! I did some experimenting on this as well, basically the same findings as Ben. Quick fix is to use This definitely has to do with escape codes, but it is also hard for me to debug to see how dune is affecting the escape codes. Due to their own color detection turning off colors when you start piping it places, and because it's using stderr not stdout. For example I can see exactly how Rely prints with color by using this command:
But when trying with
You can see all the color codes are removed already, even though there should be some in the output if dune didn't detect these pipes weren't a terminal. I figured the If you're able to play around with it and see exactly how dune is messing with the error codes we can debug it a bit further and it would probably help in setting up a custom printer/reporter to work with dune better. |
Oh actually I was able to get the output, it is definitely doing something crazy with escape codes: (Mac version of calling script to fake a tty according to stack overflow)
|
I think I see what dune is trying to do. It looks like it is trying to shorten/optimize the escape codes but doing so incorrectly, if you look at dune's (top) compared to corresponding normal escape codes (bottom):
Then it looks like in the next instance of "optimizing" it forgets to stop adding
|
Okay I see what the issue is, I'll file a bug against dune. It keeps reapplying cursor movement escape codes. In general what it's doing is not terrible. It collected error codes that apply to each section of text, resets them, then reapplies them all. I was missing the reset error codes from before:
Then:
In general this would be fine, but we use control sequences to move the cursor and delete text ( |
Hopefully they can fix this issue in dune: ocaml/dune#3160 It may also be worth adding a feature to Rely to not print any side-effect escape codes with cursor movement/line deletion. |
That's awesome @kyldvs, thanks a lot for looking into this! I can look into adding a feature to remove these escape codes. Do you have any recommendation on how to implement it? Would it be part of another reporter? |
Yeah I think you could almost use exactly the same TerminalReporter. I think what needs to happen is we disable all of this (maybe needs to be replaced with new lines instead) Since everything else can basically be the same, we can probably just extend the Then enable it in the CLI via some option: https://github.com/facebookexperimental/reason-native/blob/master/src/rely/RelyCLI.re#L18-L52 |
@tmattio It looks like another workaround can be to use (I don't know how to use base, but you want an extra newline before calling into Rely because dune prints some things first) |
Hi @kyldvs! Sorry for the delay. I've tried this solution and it works really well, thanks a lot for deeping into this, it helped a lot! I'm happy with this solution for my use case. Do you think Rely would still benefit from a custom reporter? |
Yes I think it could still be beneficial! Perhaps as another format for CI or other non-interactive places. (Also I believe along with |
Got it, I'll try to look into it 🙂 |
I'm trying to use Rely in combinaison with
dune runtest
.Here's my dune file:
When I use the test stanza and run
dune runtest
, the output format looks like this:It's hard to see what's failing.
In comparison, the output for the same test suite when using a normal executable looks like this:
Is there anything I can do to improve the output format with dune?
Thanks!
The text was updated successfully, but these errors were encountered: