haf / expecto

A smooth testing lib for F#. APIs made for humans! Strong testing methodologies for everyone!
Apache License 2.0
668 stars 96 forks source link

Console Color not reset? #249

Closed matthid closed 6 years ago

matthid commented 6 years ago

From time to time I see https://github.com/fsharp/FAKE/issues/1858 in my shell. And somehow I feel like expecto is to blame here.

Regarding

https://github.com/haf/expecto/blob/04f12c07e7d148c8a7204e7bc6badc73648c5f46/paket-files/logary/logary/src/Logary.Facade/Facade.fs#L709-L720

Is it possible that multiple LiterateConsoleTarget instances exist for example? Or that the execution is cancelled because another test has failed and it doesn't reset the color in that scenario? Is there anything that comes to mind?

adamchester commented 6 years ago

Yes, it's quite likely there are either multiple instances using different semaphore locks, or possibly even just loggers being written to async.

Unfortunately there's no easy way to make the coloured output "atomic" across multiple console writers using these APIs (here, we can only serialise writers that use the same lock object, the sem argument). This is because of the way the Console.ForegroundColor and related APIs work.

A good option to work towards is to move away from the System.Console APIs for colours and instead use ANSI console colour codes. This way, the entire line written to the console TextWriter.WriteLine(string), along with embedded colour codes, is written atomically. Even Windows 10+ and VSCode etc are supporting this now.

However, until that happens, the best we can do is make sure we serialise console writers.

matthid commented 6 years ago

Note that if Console.Write throws (don't know if there is a scenario where that happens) or some other exception happens than the console color will not be reset either.

Also note that I'm not really too much concerned about wrong colors within the run. All I currently care about is that expecto should leave my shell in a sane state :/

haf commented 6 years ago

See #248 - I think the case is that we're not waiting for the console printer to finish

matthid commented 6 years ago

Can I somehow workaround?

haf commented 6 years ago

You can change all info etc to infoWithBP in this repo and PR that and I'll release it. Generally I think I need to change https://github.com/logary/logary/blob/master/src/Logary.Facade/Facade.fs#L216 into https://github.com/logary/logary/blob/master/src/Logary/LoggerModule.fs#L33 — or what do you think is the right semantics for Async?

matthid commented 6 years ago

I have no idea what you are talking about :) The only thing I did is search for ForegroundColor and the result looked suspicious. There is no info or infoWithBP in the two links?

haf commented 6 years ago

Yes there is in the first link

matthid commented 6 years ago

So it will no longer be async basically?

haf commented 6 years ago

Yes, it being async means it's not blocking your output (which means in turn it doesn't reset the colour)

haf commented 6 years ago
screen shot 2018-05-25 at 11 27 48

This is what the output looks like in Logary proper; we have a different way of handling data there. I think we should have a design session around how to handle modern terminals and how to deal with structured output (and not just structured logging/data of messages).

Also, we should move to ANSI escape sequences, opted out automatically for terminals it's known not to work with.

Here's the PR for the new Facade https://github.com/logary/logary/pull/335

AnthonyLloyd commented 6 years ago

Yes there are a few console issues that should be addressed together. Would be good to perfect the output.

I'm in the highlands of Scotland next week so may not be so easy to stay in touch.

AnthonyLloyd commented 6 years ago

resolved with #258