google / googletest

GoogleTest - Google Testing and Mocking Framework
https://google.github.io/googletest/
BSD 3-Clause "New" or "Revised" License
34.52k stars 10.09k forks source link

[Bug]: --gtest_color=yes ignored when no filesystem supported #4439

Closed juliencombattelli closed 8 months ago

juliencombattelli commented 9 months ago

Describe the issue

When building GoogleTest with no filesystem support (primarily for bare-metal targets), the command line argument --gtest_color=yes has no effect and the output is not colored. Even if the output stream is not a TTY, when I explicitly require colors in the output using --gtest_color=yes I would expect to see the ANSI color codes emitted.

Steps to reproduce the problem

The following example was tested on Ubuntu 22.04 but should have the same behavior on any Linux system.

1- Clone googletest repo in a directory

git clone https://github.com/google/googletest

2- Create the following files next to the googletest folder

add_subdirectory(googletest) target_compile_definitions(gtest PUBLIC GTEST_HAS_FILE_SYSTEM=0)

add_executable(GoogleTestNoFilesystem main.cpp) target_link_libraries(GoogleTestNoFilesystem PRIVATE gtest)

  - main.cpp
```C++
#include "gtest/gtest.h"

int main(int argc, char* argv[])
{
    testing::InitGoogleTest(&argc, argv);
    return RUN_ALL_TESTS();
}

3- Build the code

cmake -S . -B build && cmake --build build

4- Execute the generated binary

image

image

What version of GoogleTest are you using?

dddb219c3eb96d7f9200f09b0a381f016e6b4562

What operating system and version are you using?

Ubuntu 22.04

What compiler and version are you using?

Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/11/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none:amdgcn-amdhsa
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 11.4.0-1ubuntu1~22.04' --with-bugurl=file:///usr/share/doc/gcc-11/README.Bugs --enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++,m2 --prefix=/usr --with-gcc-major-version-only --program-suffix=-11 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --enable-bootstrap --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-plugin --enable-default-pie --with-system-zlib --enable-libphobos-checking=release --with-target-system-zlib=auto --enable-objc-gc=auto --enable-multiarch --disable-werror --enable-cet --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-offload-targets=nvptx-none=/build/gcc-11-XeT9lY/gcc-11-11.4.0/debian/tmp-nvptx/usr,amdgcn-amdhsa=/build/gcc-11-XeT9lY/gcc-11-11.4.0/debian/tmp-gcn/usr --without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu --with-build-config=bootstrap-lto-lean --enable-link-serialization=2
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04)

What build system are you using?

cmake version 3.27.0

Additional context

This issue could be fixed by using ShouldUseColor() either a filesystem is supported or not:

diff --git a/googletest/src/gtest.cc b/googletest/src/gtest.cc
index 479b2ee3..39f9aeec 100644
--- a/googletest/src/gtest.cc
+++ b/googletest/src/gtest.cc
@@ -3282,12 +3282,13 @@ static void ColoredPrintf(GTestColor color, const char* fmt, ...) {
   va_list args;
   va_start(args, fmt);

-  static const bool in_color_mode =
+  static const bool in_color_mode = ShouldUseColor(
 #if GTEST_HAS_FILE_SYSTEM
-      ShouldUseColor(posix::IsATTY(posix::FileNo(stdout)) != 0);
+      posix::IsATTY(posix::FileNo(stdout)) != 0
 #else
-      false;
+      false
 #endif  // GTEST_HAS_FILE_SYSTEM
+  );

   const bool use_color = in_color_mode && (color != GTestColor::kDefault);

Would it be an acceptable solution? I can create a PR for this patch if it is ok.