Implement Google Test framework #1126
Conversation
nmoinvaz
added
cleanup
nmoinvaz
force-pushed
the
improvements/google-test
branch
from
3069af5 to
e53d554
Compare
|
@KungFuJesus with Google Test, when running in Debug mode, using MSVC I get those AVX512 Adler32 errors the CI has seen. Only happens in Debug mode, Release mode is ok.
|
Codecov Report
@@ Coverage Diff @@
## develop #1126 +/- ##
===========================================
+ Coverage 87.09% 87.45% +0.36%
===========================================
Files 96 112 +16
Lines 9149 9608 +459
Branches 2258 2429 +171
===========================================
+ Hits 7968 8403 +435
+ Misses 1063 961 -102
- Partials 118 244 +126
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
nmoinvaz
force-pushed
the
improvements/google-test
branch
from
e0d904e to
c64fc2c
Compare
So I fixed an issue that can happen due to not properly zero extending the 128 bit registers to 512 bit. Are you on the latest version of this PR? The zextsi128 stuff should prevent that from happening. Ohh, it just occurred me you probably are implementing this on top of develop. This will go away once that PR goes through. What's happening is the _mm512_castsi128_si512 allows the top half the register to be undefined unless you do an explicit 128 bit move first. For the benefit of tracking, this is the PR: #1121. |
|
@KungFuJesus I am also now seeing the problem in AVX2 on CI. Before it was only AVX2-VNNI. Hopefully your PR fixes both. I could cherry-pick your PR into my branch to see if it helps. |
It definitely should, the proper zero extension is being done in both. It was just an unfortunate circumstance of it working enough that the undefined nature wasn't caught. All part of my favorite t-shirt: https://my-store-be64e.creator-spring.com/listing/undefined-behavior-shirt? |
|
|
It's not just about possibility of encountering too old C++ compiler, but also the C++ compiler is kind of optional separate package on a lot of distributions and as the core code doesn't need it, it should not be changed as mandatory build dependency. |
|
Windows doesn't even come with C compiler |
It doesn't come with C++ compiler either... Technically the API is plain C, but the compiler is C#, but it can also compile hybrid C++/C# code. Since .NET Framework was changed as mandatory build requirement for Windows, the Visual Studio compiler has been C#. Some people say CL is short for Common Language. |
I'm pretty sure that microsoft has a plain jane C++/C compiler that is separate from the rest of the .NET VM. What microsoft refers to as "unmanaged code" is the vanilla C++ compiler. Managed code compiles to IL and then turns into .NET bytecode via some common language runtime. Native, or unmanaged code I think interacts with the .NET VM through cdecl calling convention with standard foreign function interfaces. At least that's what I can recall when dealing with it from C# on Linux. Native code very much still exists, I definitely wouldn't want to write a driver that depended on the .NET VM. |
I've compiled enough C++ applications under Windows at work that I know all except very simple Hello World type applications need CL runtime to even execute correctly. You get cryptic error code if the run-time is missing or incompatible version. Because the run-time is delay-loaded, it doesn't show up as dependency using normal dependency walker unless it allows attaching debug hooks. |
|
I'm guessing/hoping that this fixes the "code coverage" in a way that tells us reliably that we're checking everything, at least on nodes that can execute it. Though, the runtime determined support checks may trigger codecov to say "hey, there's a good chance we won't execute this all the time". It'd be useful from the standpoint of, "will this get tested by the unit tests, and if not, we should write a new test" sort of deal. |
|
I've been continuing work on a different branch on my repo so I don't spam zlib-ng CI pipeline or this PR. There are some other changes that I need to get in first that I will make a PR for. |
|
Configure running less stringent tests than CMake is OK I think, but it would be nice with a documented way to get Google Test running manually without CMake though, if possible. If the user has to download things themselves etc, then that is fine. One possible issue I see with this is it requires CMake 3.12, and anything older than that will not be running the tests. IDK, the above is not optimal, but is it acceptable? |
|
For pkgcheck test to pass, it must skip all the files and directories installed by Google Test. |
|
Can you tell please why did you choose Google Test framework? Google Test is quite difficult to integrate in the project. Maybe use the frameworks:
Catch 2 and doctest require adding a single header file to the project. |
|
It is the one I was most familiar with and since I also added Google Benchmark it was an easy extension of that to use Google Test. Anything that is C++ is going to make integration difficult because we haven't really had to deal with CXX compiler until now. It is possible to use locally downloaded sources, however one thing about keeping the sources separate is there are no licensing issues as we try to keep only zlib licensed code in the repository. I'm not interesting in reworking everything I've done to use another test framework. |
nmoinvaz
force-pushed
the
improvements/google-test
branch
from
9d024ab to
ed185a8
Compare
|
The remaining CI failures are:
Not sure how easily these can be overcome. |
nmoinvaz
force-pushed
the
improvements/google-test
branch
from
e51e7bd to
427d745
Compare
|
Rebased. |
|
I was thinking keep test_ targets mainly do functional testing that tells a user that it is working correctly/compiled right etc. While the cve and fuzzer targets test things that are more useful for us as maintainers, regression tests etc, and should not crop up on a users compile without us already knowing about it. |
|
Oh, and keep example too ofc, since it is the basic functionality testing. |
nmoinvaz
force-pushed
the
improvements/google-test
branch
from
d45d7d9 to
8d8b761
Compare
|
The I have removed the commit that removed |
nmoinvaz
force-pushed
the
improvements/google-test
branch
from
8d8b761 to
3d3060c
Compare
|
We can't remove |
|
This fails to compile on EL7 I think we would need to incorporate the patch mentioned earlier: --- _deps/googletest-src/googletest/include/gtest/gtest-matchers.h.old 2022-03-07 14:27:56.484777293 +0100
+++ _deps/googletest-src/googletest/include/gtest/gtest-matchers.h 2022-03-07 14:23:27.514498802 +0100
@@ -242,6 +242,14 @@
T value;
};
+template <typename T>
+using is_trivially_copy_constructible =
+#if defined(__GNUC__) && !defined(__clang__) && __GNUC__ < 5
+ std::has_trivial_copy_constructor<T>;
+#else
+ std::is_trivially_copy_constructible<T>;
+#endif
+
// An internal class for implementing Matcher<T>, which will derive
// from it. We put functionalities common to all Matcher<T>
// specializations here to avoid code duplication.
@@ -415,7 +423,7 @@
template <typename M>
static constexpr bool IsInlined() {
return sizeof(M) <= sizeof(Buffer) && alignof(M) <= alignof(Buffer) &&
- std::is_trivially_copy_constructible<M>::value &&
+ is_trivially_copy_constructible<M>::value &&
std::is_trivially_destructible<M>::value;
}
So, should we patch that file locally? Perhaps just run sed to replace the function name if GCC is 4.x? It is unfortunate that gtest (and other google projects) are not really made for use outside of Google, and as such they don't consider portability and backwards compatibility to be as important (for example Snappy is rejecting patches that fix compilation on other platforms). Gtest does not support GCC older than 5.3, ref google/googletest#3444 and google/googletest#3387 |
|
I have pushed a commit that will use older version of Google test when GCC <= 5.3. @Dead2 can you give that a try? |
nmoinvaz
force-pushed
the
improvements/google-test
branch
from
45e842a to
daba3f9
Compare
|
Rebased after #1161. Fixed crc32 pclmulqdq tests and fixed cpu check features not being called. |
|
Unfortunately this now fails with the following error. I assume it is a missing new feature in the older gtest.. /usr/src/github/zlib-ng/test/test_adler32.cc:354:25: error: expected constructor, destructor, or type conversion before ‘(’ token
INSTANTIATE_TEST_SUITE_P(adler32, adler32_variant, testing::ValuesIn(tests));
^
/usr/src/github/zlib-ng/test/test_adler32.cc: In member function ‘virtual void adler32_variant_c_Test::TestBody()’:
/usr/src/github/zlib-ng/test/test_adler32.cc:359:24: error: ‘GTEST_SKIP’ was not declared in this scope
GTEST_SKIP(); \
^
/usr/src/github/zlib-ng/test/test_adler32.cc:365:1: note: in expansion of macro ‘TEST_ADLER32’
TEST_ADLER32(c, adler32_c, 1)
^ |
nmoinvaz
force-pushed
the
improvements/google-test
branch
from
daba3f9 to
a4281c4
Compare
Allow specifying alternative Google test repository and tag.
nmoinvaz
force-pushed
the
improvements/google-test
branch
from
a4281c4 to
3d420eb
Compare
|
Actually, I think the only other |
|
@nmoinvaz Confirmed, 1.10.0 works just fine. Good find 👍 |
|
That's great news. I have removed the googletest fork. |
example.example, but just use simpleminigziptests. This is because fetching Google Test repository might add more complexity than it is worth.