-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Add "ansi" to supported gtest_color values #4050
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
5361203
to
b10aafc
Compare
Instead of adding an API for forcing ANSI, could we detect whether the output terminal supports it or not? |
I doubt it's possible on Windows. If it does not run on Windows, it uses If it runs on Windows, it is able to detect if the output terminal supports colors. But it does not work if the output stream is redirected to a file or a pipe, what exactly happens if
|
@@ -74,6 +74,8 @@ GTEST_DECLARE_bool_(death_test_use_fork); | |||
namespace testing { | |||
namespace internal { | |||
|
|||
enum class GTestColorMode { kNo, kYes, kAnsi }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried about the difference between yes
and ansi
. Both of these are used to turn on colors, but in different ways. If we were to design GoogleTest from scratch, we'd probably want to provide a flag with possible values such as ansi
, no
, auto
, and not_ansi
(probably something more informative for the last one).
Instead of expanding the existing flag, could we add another one for using ANSI color escape sequences? I'm okay with it being a force flag (on/off) or a feature flag (on/off/auto).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The enum is made with the logic 0 - coloring is disabled, !0 - coloring is enabled, 1 or 2 - it's a detail how it is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could make another flag --gtest_color_ansi
and keep --gtest_color
unchanged, but did not want to add yet another environment variable and to make a larger changeset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend adding another flag, e.g. something like --gtest_use_ansi_color
and keep the existing one unchanged.
Most users shouldn't care about the new flag, but by providing it, we leave an escape hatch for those that fail to auto-detect ANSI colors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep the existing one unchanged.
The behavior of the flag --gtest_color
and the environment variable GTEST_COLOR
with the existing values is unchanged. Nothing is changes for the existing use cases, most users shouldn't care about the new flag - it is kept so, until they start using the new value ansi
.
I believe #3049 fixes this problem without introducing any options. |
Unfortunately it does not fix the coloring issues.
Neither above is able to color the output. Running them in Windows PowerShell additionally produces UTF-16 uncolored outputs. #3049 is showing the working redirection |
@sergio-nsk I've now rebased #3049 on the latest Which compiler are you using on Windows? My PR has the logic only for MinGW toolchain, although I contemplated generalizing it to all Windows compilers. I've also confirmed the color output (using ANSI escape sequences) working in a PowerShell window with both |
2aebc2f
to
e556843
Compare
Fixes uncolored output on Windows PowerShell with CTest (google#2930) Fixes uncolored output on GitHub and other Windows terminals. CTest writes googlest standard output and error streams to standard output and a file. Windows `_isatty()` returns 0 in this case and `ShouldUseColor()` returns `false`. Windows PowerShell supports ANSI escape sequences.
e556843
to
45d2620
Compare
Fixes uncolored output on Windows PowerShell with CTest (#2930)
Fixes uncolored output on GitHub and other Windows terminals.
CTest writes googlest standard output and error streams to standard output and a file. Windows
_isatty()
returns 0 in this case andShouldUseColor()
returnsfalse
. Windows PowerShell supports ANSI escape sequences.Reviewed-by: Nathan Moinvaziri nathan@nathanm.com
Reviewed-by: steve-tucker steve-tucker@users.noreply.github.com