Add Google Test as the new test framework #190
Conversation
|
@johnkslang I've just converted two existing tests as a demo. PTAL and share your opinions on this approach. It turns out with |
|
@johnkslang The tests are copied exactly from the original test files. The expected outputs are also copied directly from |
| @@ -47,6 +47,11 @@ CMake: The currently maintained and preferred way of building is through CMake. | |||
| In MSVC, after running CMake, you may need to use the Configuration Manager to | |||
| check the INSTALL project. | |||
|
|
|||
| Glslang is migrating to the [Google Test](https://github.com/google/googletest) | |||
There was a problem hiding this comment.
"migrating to" might be a little strong.
How about "is adding the ability to test with "
|
-1 I like gtesting a lot, but I think this particular use makes the test and update flow worse. I think moving the source and base result text into C++ source text makes for a bad update flow. I'm all for gtesting, but the place it will add lots of value is for testing low level things, e.g. in the preprocessor. For example a fix for #29 could have unit test to ensure that the root cause (an underflow on an array index in the preprocessor) is resolved. |
|
Totally agreed with using gtests to do unit test kind of thing for future tests. But I have different thoughts on the update flow. It may be slightly worse to update than the current approach, but the problem is, we shouldn't keep adding new stuff to the existing tests. Instead we should create new test cases using gtest. Using files basically encourages (well, compared to using gtests) devs to just append things they want to check to existing files. So by "updating", it is supposed to only be bugfixing, which I assume will typically not trigger a dramatic output change. So copying from the gtest output and pasting it into the source shouldn't be a big burden. On the other side, we get the the benefits of fast loading (strings inside the program vs. opening files and reading), easy testing in out-of-source-build scenario (remember the pain we have in order to do clean out-of-source testing in shaderc?) better contrast (with input and expected output quite near each other, instead of traversing filesystem to open different files), etc. I think the benefits outweigh the loss in update flow. Ideally, some of these large integration tests should be splitted into multiple minimal tests covering only one features to be tested. But as a introducing-new-test-framework thing, it would be difficult to review by doing that. So, next step maybe. |
|
I figure we ought to load test inputs and outputs from files. It seems it would be annoying changing the test source code to run with different test data; especially given that they are plain strings. |
|
@antiagainst Wow, thanks. We definitely need a way to have lots more and smaller tests without running slower than the current suite. We also need a fast and smooth workflow for
That can currently be done from the Test directory by
Now, there are really two projects here, that have different histories and needs, and hence its possible the action to take WRT testing today might be different:
Note the differences between them: GLSL validator
SPIR-V generator
Main point: Growing a new fresh complete set of SPIR-V tests would be sensible and valuable, while not losing anything from the GLSL validator semantics tests is critical. SPIR-V generation is actually getting better testing, through execution, through the Khronos CTS suite execution on actual drivers. So, the ideal thing to do for the validator might be different than the ideal thing to do for SPIR-V generation. No matter what though, each test suite is subject to a change that can slightly effect almost all outputs, and after verifying the test results changes are okay, it should be trivial to mass update all the test results. I suspect most performance issues are with creating multiple processes, and not with doing file I/O. So, I suspect there is a solution lurking that simultaneously achieves the following objectives:
Thanks for contributing to this! |
antiagainst
force-pushed
the
gtest
branch
3 times, most recently
from
fa7a9be to
21e4875
Compare
|
Still WIP.
Example of running: |
|
@dneto0 @xorgy It makes sense for me now, thanks for the suggestion! I've converted to loading from files. :) @johnkslang: Thanks for the great detailed explanation of the scopes and high-level thingy. Now I see the big picture more clearly. Yeah, it would be better to avoid touching the stable GLSL tests. Future SPIR-V tests and unit tests better go with gtest. |
|
@antiagainst Looking great. :-) |
|
Added another patch to do real per process initialization. This significantly reduces time required to run tests. (See the commit msg for details.) |
| GlslangEnvironment() { glslang::InitializeProcess(); } | ||
| ~GlslangEnvironment() { glslang::FinalizeProcess(); } | ||
| }; | ||
| GlslangEnvironment environment; |
There was a problem hiding this comment.
The Google style guide forbids variables of (non trivial) class type with static storage duration, due to concerns about indeterminate order of construction and destruction.
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
You can achieve the same thing by putting the initialization and finalization right into the main() function.
There was a problem hiding this comment.
Added some comments to explain why this happens.
|
Looking pretty good. Thanks!
|
|
Has anyone taken a look at how Clang/LLVM do testing? Their test suite is pretty sophisticated. When it comes to Clang they have several stages of testing. First there are usual unit tests, but they really just test classes and functions, they are just your average unit tests that don't touch any files. Second Clang can run in verification mode with a hidden command line switch int x{2.f}; // expected-error {{narrowing conversion from 'float' to 'int'}}
int y = x; // expected-warning {{unused variable 'y'}}At the end of the translation unit Clang checks that the diagnostics it created match what's written in the comments and if not the compilation fails with the appropriate error message indicating which tests failed. If all encountered errors match the expected errors the compilation is considered successful. And third, for positive testing they have a tool called FileCheck that they pipe the compiler output and input to to ensure the file has the expected content. For example // RUN: glslangValidator -V -o - %s | FileCheck %s
void main()
{
// CHECK: [[INT:%[0-9]+]] = OpTypeInt 32 0
// CHECK-DAG: [[ONE:%[0-9]+]] = OpConstant [[INT]] 1
// CHECK-DAG: [[TWO:%[0-9]+]] = OpConstant [[INT]] 2
// CHECK: [[C:%[0-9]+]] = OpVariable [[INT]] Function
// CHECK: [[TMP:%[0-9]+]] = OpIAdd [[INT]] [[ONE]] [[TWO]]
// CHECK: OpStore [[C]] [[TMP]]
int c = 1 + 2;
}(The This is all obviously more work, even though the FileCheck tool already exists and could be borrowed from LLVM. I think the morale of this story is that the input and expected output are always in the same file for easier maintenance. They also support the same input be compiled several times with different options and the More details about FileCheck is here: http://llvm.org/docs/CommandGuide/FileCheck.html |
|
@mknejp: FileCheck is definitely a great tool for solving text-based flexible comparison. However, as @johnkslang mentioned previously, there are two projects involved in this codebase: one for GLSL validator and one for SPIR-V generator. I'm not sure adopting FileCheck for the former, which means to rewrite all the "stable" tests, is the way to go. And we want unit tests for the both projects. FileCheck cannot do that, but GTest can. Even if we use FileCheck for the SPIR-V generator, we still need to pull in GTest for unit testing. Now we have three testing harnesses, which is not quite optimal. Besides, FileCheck doesn't provide a smooth update workflow, I think. By using GTest, we have the ability to grow future unit tests. And by hacking GTest a little bit, we can provide the same (or better) functionality than the But, yeah, this is just my thoughts. Definitely needs @johnkslang's expertise on this issue. |
|
@antiagainst I'm not saing abandon unit tests, but use unit tests what they are meant for. Right now, unless I am mistaken, we pretty much assume that the SPIR-V disassembler is error-free when doing the diff-based tests. What we really have to test is the actual binary produced by When it comes to the GLSL validator project I agree that FileCheck is of little use. That's why Clang has the Not sure what you mean by a lack of smooth update process. The FileCheck based process is you define the output you expect to be in the generated file and because these tests can be made much shorter and more focused than a full-blown SPIR-V dump they are much less sensitive to changes in instruction reordering or different counting of IDs or other generator changes. You can have one isolated test for every expression, statement or feature and make sure it generates the opcodes you want in the order you want/don't want. There shouldn't be much of an upgrade process, only adding new tests. The way it is right now, whether with gtest or file diffs, if you change the order in which IDs are generated in SPIR-V you have to update all test files because suddenly the ID of OpEntryPoint is different in every single one of them. The tests are not isolated. A test should not fail because I changed something it is not testing. Worse, a human has to go over all those changed files and manually check that their semantics have not changed. That is very scary. |
|
@mknejp Sorry, I was reading too quickly and only focused on the latter parts of your arguments, so misunderstood your idea. I thought you were suggesting to use FileCheck instead of GTest. :) This patch is to add GTest as a framework for future unit tests. And it would also be nice to have a better mechanism to replace the current Unit testing part is not that controversial, so you see what I'm doing here, for the purpose of seeking feedback, is just providing a kinda hacky (yeah, I know and admit this) way of subsuming the responsibility of The id-agnostic diff is a nice thing to have, and clearly FileCheck is a nice way to go, but that would be a different issue if we have the assumption of using different test harnesses to do what they are good at. For the workflow thing, my previous discussion is also targeting at existing tests. |
|
@mknejp : I agree there's a lot more we can do to test glslang at various levels of granularity. Yes, I've worked with Clang and LLVM extensively, writing many tests for it and understand its benefits. They're great, but we don't have similar tools here (yet?). This merge request is primarily about adding the ability to use gtest at all. Gtest really shines for unit tests, clearly. For example, once I can test with Gtest, I'd like to fix #29 but do it with confidence by adding unit tests. |
antiagainst
force-pushed
the
gtest
branch
2 times, most recently
from
081f82d to
72e8ab3
Compare
But now this is just a demo of concept (for seeking feedback before I go too far) and only tested out on Linux. So please just wait a minute, I'll go and test this out on Windows and makes sure it works fine. ;) (Line endings, etc. could be a problem when comparing results.) Well let you know when I think it's ready. Thanks! |
|
Ready to be merged right now. :) How to try out: place a copy of GTest under How to update expected output with real output: run the |
|
For the record I just tested this change versus runtests for just the spirv/preprocessor tests and got the following results for speed: runtests: (only running spv and preprocessor tests) glslangtests.exe (only the spv and preprocessor tests) so around a 17x speed increase on Windows for the change. |
|
Awesome. Reusing symbol tables and processes is a big win, of course, as is not having a shell script forking a bunch of commands. The script was also testing parallel compiles: it compiled a bunch of shaders multithreaded to test for issues there, and that ran similarly fast. We won't want to lose that thread testing, BTW. |
|
8d380b8 adds a unit test for one of the newly added functions. :) |
|
Hi @johnkslang, the code is ready to be merged now. Tried on Windows with VS 14 2015 & MSVC 19.0.23506.0. Built & Tested cleanly. |
|
Do we know about VS 2013? We keep adjusting for 2010 in the core (required) code base, but I think it's okay if 2010 can't the extended (optional, gtest) code base. As a middle ground, I run and test frequently on 2013, and think many do, so it would be desirable for 2013 to work. |
|
I ran this, at least at the time of my last comment, on VS2013 and everything worked. |
|
We just verified the latest version of this PR on VS2013: |
|
Hi @johnkslang, how do you think about the current implementation? |
| "spv.specConstant.vert", | ||
| "spv.specConstant.comp", | ||
| // GLSL-level semantics | ||
| "vulkan.frag", |
There was a problem hiding this comment.
Note the spv.* tests are expected to compile without error, so they can go all the way through to SPIR-V without error, while most other tests, including vulkan.* tests are testing the semantics of the front-end, expected to get the right set of errors without going through to SPIR-V.
I'm unclear yet on which list then you want the vulkan.* tests in; this one or the earlier one. Or, a separate one. It does seem a bit inconsistent here, as they are the only ones in this list that need their error messages checked, rather than expected to get no errors.
There was a problem hiding this comment.
Ah, previously I didn't notice there are such differences: just did a bindly copy-pasta of the Test/test-spirv-list file. ;)
The way how these in-file tests are loaded, compiled, and then checked is coded via LoadFileCompileAndCheck() & CompileGlsl(). Basically, it just mimics how the standalone glslangValidator goes: compiling and linking shaders, and concatenating InfoLog and DebugInfoLog from both compiling and linking and comparing them with golden files. So no matter whether errors are expected or not, the procedure is the same.
To check different output channels for different existing in-file tests, I believe the expected result files need to be modified to remove the output from other channels; but that will cause problem with the runtests script when running multiple-threaded tests. (One of the purpose is to handle the responsibility of single-threaded tests from the runtests script to GTests, but not multiple-threaded tests for now.)
I did a rebase against master with a new commit a80673a, in which a separate template is created for the vulkan.* tests to discriminate them.
|
|
||
| ${CMAKE_CURRENT_SOURCE_DIR}/Spv.General.cpp | ||
|
|
||
| ${CMAKE_CURRENT_SOURCE_DIR}/main.cpp |
There was a problem hiding this comment.
No need for main.cpp -- use gtest_main target like in shaderc/cmake/utils.cmake:7.
There was a problem hiding this comment.
But we need an extra cloption --update-mode, which is added in main.cpp.
antiagainst
force-pushed
the
gtest
branch
2 times, most recently
from
3d4dbfb to
af6e729
Compare
|
Looks like things are essentially ready, just need to squash+rebase the commits and do a final review. :-) |
The existing test harness is a homemade shell script. All the tests and the expected results are written in plain text files. The harness just reads in a test, invoke the glslangValidator binary on it, and compare the result with the golden file. All tests are kinda integration tests. This patch add Google Test as an external project, which provides a new harness for reading shader source files, compile to SPIR-V, and then compare with the expected output.
|
Hi @johnkslang, the code is squashed and rebased. Ready to go in now. Thanks! |
Header generator: Check enumerant ordering
Addresses #189.
This patch add Google Test as an external project for future unit tests and subsuming the responsibility of
runtests.