-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Test Suite #1345
base: dev
Are you sure you want to change the base?
[WIP] Test Suite #1345
Conversation
19539ed
to
b94141d
Compare
2cbbf82
to
8289a2f
Compare
A suggestion since you are currently working on it: I think now it would be better to put tests in |
.github/workflows/ccpp.yml
Outdated
export PKG_CONFIG_PATH="$CUSTOM_BREAKPAD_PREFIX/lib/pkgconfig:${PKG_CONFIG_PATH:-}" # | ||
export PKG_CONFIG_PATH="$CUSTOM_BREAKPAD_PREFIX/lib/pkgconfig:${PKG_CONFIG_PATH:-}" | ||
export QT_QPA_PLATFORM=minimal | ||
export LD_LIBRARY_PATH=./Rizin-prefix/lib:$LD_LIBRARY_PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is LD_LIBRARY_PATH still necessary? At least in my local build CMake was setting RPATH correctly that build output is runable without modifying LD_LIBRARY_PATH.
I have mixed feelings about the way AutoTest.h handles multiple test classes. On one hand I am not too happy about idea of an executable per test files, on the other it breaks QtCreator integration and some of the Qt Test framework functionality. Haven't checked how it affects Visual Studio and KDevelop integration. Since DECLARE_TEST it is still necessary having everything in single executable doesn't save any work when creating a new test. Running all the executables can be handled using CMake. Biggest potential downside of multiple test executables is the performance impact of running a bunch of executables. Since it is one executable per test class not test function it shouldn't be as bad as what happens when using |
15fb785
to
cfd336d
Compare
Fix #1263