-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix TTY color support #1805
Labels
Milestone
Comments
Somewhat connected issue: #1748 |
Found something that might be useful on HN: https://notes.burke.libbey.me/ansi-escape-codes/ (https://news.ycombinator.com/item?id=26011198) |
Closed by #1975 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This used to be considered a minor issue and lived as this code
TODO
: https://github.com/loadimpact/k6/blob/c00a85bfea21d748f0f9d11fdbc326087f34c411/cmd/root.go#L139-L154But it turns out that the problem is a bit bigger than that, evidenced by my troubles with the recent summary work (After #1788, #1768, #1803). I noticed that in the above code, we don't even check for
stdoutTTY
orstderrTTY
. Instead, we actually somewhat implement theTODO
above, but in a nasty way that doesn't use dependency injection and is completely untestable...The
color
library directly uses Go'sos
package to check things for itself: https://github.com/loadimpact/k6/blob/c00a85bfea21d748f0f9d11fdbc326087f34c411/vendor/github.com/fatih/color/color.go#L20-L21And we also directly use
stderrTTY
in our logger constructor: https://github.com/loadimpact/k6/blob/c00a85bfea21d748f0f9d11fdbc326087f34c411/cmd/root.go#L309And for a taste of #883,
nocolor
,stdoutTTY
, andstderrTTY
are all global variables... 😭 So yeah, all of these things get in the way of handling colors in the JS-based summary handlers properly...The text was updated successfully, but these errors were encountered: