Proposed testing framework for Standard Library facilities #1
Conversation
ldionne
force-pushed
the
main
branch
6 times, most recently
from
407f64b to
ca3be14
Compare
This "testing framework" is based on the LLVM Integrated Tester, which is used to implement the libc++ conformance test suite. In turn, this conformance suite is also used by other non-LLVM implementations to check their conformance. Using this testing framework means that papers contributed to the Beman project will already include tests that can be reused by the major implementations, which is both a great time saver for implementers but also a great way for implementers to get experience with implementing the paper within their own implementation and provide feedback to LEWG during design reviews.
| @@ -0,0 +1,91 @@ | |||
| import lit | |||
There was a problem hiding this comment.
For information, I am working on some LLVM changes that would render this file unnecessary. Basically we would just install Lit and the Lit package would contain exactly the same test format as the one used by libc++.
There was a problem hiding this comment.
Where are you at with these changes?
| add_test( | ||
| NAME setup-tests | ||
| COMMAND "${CMAKE_COMMAND}" --build "${CMAKE_BINARY_DIR}" --target test-depends | ||
| ) |
There was a problem hiding this comment.
This is awkward. It looks like a CTest test cannot depend on a add_custom_target, so I need to do things in this super roundabout way to ensure that the test suite is set up before the tests actually start running.
There was a problem hiding this comment.
There is no guarantee that ctest will run setup-tests before example.test. It might do that by default but there is a ctest option to run the tests in a random order. Consider dropping the setup-tests test and renaming the test-depends target example.test. This would then emulate the usual pattern followed when the test is an actual executable and not a call to a python script.
$ cmake --build --target example.test
$ ctest -R example.test
But then again, that might be misleading a user to think the example.test target is an executable. I agree, it's awkward.
There was a problem hiding this comment.
CMake doesn't provide a way to indicate tests have dependencies so the prevailing practice is for tests to assume all targets part of the default target are built.
To ensure test-depends is part of the default target, you need only add the ALL keyword to the add_custom_target call.
| add_test( | ||
| NAME setup-tests | ||
| COMMAND "${CMAKE_COMMAND}" --build "${CMAKE_BINARY_DIR}" --target test-depends | ||
| ) |
There was a problem hiding this comment.
There is no guarantee that ctest will run setup-tests before example.test. It might do that by default but there is a ctest option to run the tests in a random order. Consider dropping the setup-tests test and renaming the test-depends target example.test. This would then emulate the usual pattern followed when the test is an actual executable and not a call to a python script.
$ cmake --build --target example.test
$ ctest -R example.test
But then again, that might be misleading a user to think the example.test target is an executable. I agree, it's awkward.
| flags = [ | ||
| '-std=c++20', | ||
| '-isysroot @CMAKE_OSX_SYSROOT@' if '@CMAKE_OSX_SYSROOT@' else '', | ||
| '-isystem {}'.format(os.path.join('@CMAKE_SOURCE_DIR@', 'src', 'example')), | ||
| '@CMAKE_CXX_FLAGS@' | ||
| ] |
There was a problem hiding this comment.
When the project under test is more complex, I expect that there will be transitive usage requirements that need to get passed to the compilation flags here. Do those need to be repeated manually or is there a way to query cmake?
There was a problem hiding this comment.
Another thing that needs to be verified is cross-compilation support.
|
For whatever reason, GitHub isn't allowing me to comment on @frankmiller's comment, but CMake has a way for tests to depend on one another: I'm inclined to get something here merged before we completely lose our collective context from last week. I do think we need to circle back and simplify a bit if possible, though. I want the project structure and the CMakeLists.txt in the project to be as simple and standard as possible. |
|
I agree with @bretbrownjr on the importance of a simple and standard project structure and that was the underling motivation for both of my comments above. Having said that, please don't let me block progress. I think its reasonable to move fast and refine in future PR's. |
| config.name = 'Beman project test suite' | ||
| config.test_format = testformat.CxxStandardLibraryTest() | ||
| config.test_exec_root = '@CMAKE_CURRENT_BINARY_DIR@' | ||
| config.test_source_root = '@CMAKE_CURRENT_SOURCE_DIR@' |
There was a problem hiding this comment.
Usage of CMAKE_CURRENT_BINARY_DIR and CMAKE_CURRENT_SOURCE_DIR is confusing here since these are unrelated to the location of lit.cfg.in. I'd suggest making some new variables on the CMake side and using them here.
So, this file would instead be...
config.test_exec_root = '@LIT_TEST_EXEC_ROOT@'
config.test_source_root = '@LIT_TEST_SOURCE_ROOT@'
And test/example/CMakeLists.txt would turn into something like this...
block()
# Setup the test suite configuration
set(LIT_TEST_EXEC_ROOT ${CMAKE_CURRENT_BINARY_DIR})
set(LIT_TEST_SOURCE_ROOT ${CMAKE_CURRENT_SOURCE_DIR})
configure_file("${CMAKE_SOURCE_DIR}/test/support/lit.cfg.in"
"${CMAKE_CURRENT_BINARY_DIR}/lit.cfg")
endblock()| @@ -0,0 +1,91 @@ | |||
| import lit | |||
There was a problem hiding this comment.
Where are you at with these changes?
| on top of that. However, we recommend against doing so because keeping the tests | ||
| basic makes them very portable, and in particular it makes them compatible with | ||
| the LLVM C++ Standard Library conformance test suite developed as part of libc++, | ||
| which is also reused by other implementations like the MSVC STL and libstdc++. |
There was a problem hiding this comment.
Using assert precludes test runs under the -DNDEBUG flag. Wouldn't that pose a problem?
| flags = [ | ||
| '-std=c++20', | ||
| '-isysroot @CMAKE_OSX_SYSROOT@' if '@CMAKE_OSX_SYSROOT@' else '', | ||
| '-isystem {}'.format(os.path.join('@CMAKE_SOURCE_DIR@', 'src', 'example')), | ||
| '@CMAKE_CXX_FLAGS@' | ||
| ] |
There was a problem hiding this comment.
Another thing that needs to be verified is cross-compilation support.
|
This pr seems to be stale, can we close this? |
I believe it can be closed. I don't think this got sufficient traction to move forward since people are more used to e.g. CTest, which is less flexible but more standard and easier to set up. There's also the requirement of being able to support a non GCC-like frontend which would require more work than I'm willing to put in as of right now. |
|
Yeah, you're probably right that we should close this. I do expect we'll want something that can model failures and such at some point, though. Possibly there's a compromise where we have a test structure reasonably easy to port or integrate into lit. |
This "testing framework" is based on the LLVM Integrated Tester, which is used to implement the libc++ conformance test suite. In turn, this conformance suite is also used by other non-LLVM implementations to check their conformance.
Using this testing framework means that papers contributed to the Beman project will already include tests that can be reused by the major implementations, which is both a great time saver for implementers but also a great way for implementers to get experience with implementing the paper within their own implementation and provide feedback to LEWG during design reviews.