Use gtest to execute tests with aarch64-x64-mingw changes #44
Conversation
tests/CMakeLists.txt
Outdated
| @@ -0,0 +1,47 @@ | |||
| set(CMAKE_VERBOSE_MAKEFILE ON) | |||
| set(CMAKE_C_COMPILER "$ENV{HOME}/cross/bin/aarch64-w64-mingw32-gcc") | |||
There was a problem hiding this comment.
I'd rather change this to $ENV{INSTALL_PATH}/bin/${TARGET}-gcc to be more compatible with .github/scripts/build.sh. The default of INSTALL_PATH can be $ENV{HOME}/cross.
tests/BUILD.txt
Outdated
| cmake --build build | ||
|
|
||
| it generates Arm64 Windows executable | ||
| build/aarch64_mingw_tests |
There was a problem hiding this comment.
It's quite confusing that the generated build folder contains bin folder but the actual produced binary is in the build folder and bin is empty.
tests/BUILD.txt
Outdated
| cmake --build build | ||
|
|
||
| it generates Arm64 Windows executable | ||
| build/aarch64_mingw_tests |
There was a problem hiding this comment.
So far the repository is using - as word delimiter in file names. This adds inconsistency. The same applies for file names of all the tests in this folder and for git branch names.
tests/BUILD.txt
Outdated
| cmake -S . -B build | ||
| cmake --build build | ||
|
|
||
| it generates Arm64 Windows executable |
There was a problem hiding this comment.
Why not a full sentence? (Starting with capital letter and ending with ..)
tests/CMakeLists.txt
Outdated
| varargs_test.cpp | ||
| ) | ||
|
|
||
| set_source_files_properties(test-dll.c PROPERTIES COMPILE_FLAGS "-DIMPORT_API=") |
There was a problem hiding this comment.
Can you please explain why this is required?
There was a problem hiding this comment.
Good question. It is not needed for now after excluding dll-test from testing.
tests/bigdata_test.cpp
Outdated
|
|
||
| TEST(Aarch64MinGW, BigDataTest) | ||
| { | ||
| printf("main\n"); |
There was a problem hiding this comment.
Those printfs are useless for the gtest tests. On the other hand, I'd expect a series of EXPECT_* calls at the end of this function (fixture method). I am affraid that without them, the tests would pass even if there would be an error.
There was a problem hiding this comment.
Agree. As it was mentioned in another comment it will be done in second iteration.
tests/bigdata_test.cpp
Outdated
There was a problem hiding this comment.
Test files naming is inconsistent with naming of examples.
There was a problem hiding this comment.
because examples and tests have different purpose and most probably they will be contain different sets later, maybe it is fine to have different naming?
There was a problem hiding this comment.
Different naming is IMO OK, I am asking for keeping it consistent with the overall repository codestyle.
tests/CMakeLists.txt
Outdated
| enable_testing() | ||
|
|
||
| add_executable( | ||
| aarch64_mingw_tests |
There was a problem hiding this comment.
This creates executable without the .exe extension. It needs to be renamed manually to execute it.
There was a problem hiding this comment.
yeah, I was thinking about it and I was not sure what will be right extension for executable. it will be changed.
tests/pdata_test.c
Outdated
|
|
||
| if (SymGetSymFromAddr64(hp, offset, &dwDisplacement, p)) | ||
| { | ||
| printf(" %s+0x%.04llX", p->Name, dwDisplacement); |
There was a problem hiding this comment.
This test is heavily versbose, we should disable printing the standard output, at least for this test.
Totally agree. On the first step tests integration with gtest it will be good to know that tests are executed fully without crash on Arm64. Definitely tests should be refactored and extended by extra checks. As it was discussed it will be second iteration based on gnu packages testing. |
| Building aarch64-w64-mingw32 tests on Linux or WSL | ||
|
|
||
| cmake -S . -B build | ||
| cmake --build build |
There was a problem hiding this comment.
It would be handy to add the build folder to .gitignore.
No description provided.