Fix Unittest on Launchpad by partially reverting #15656#15673
Conversation
| # Default to offscreen rendering during tests. | ||
| # This is required if the build system like Fedora koji/mock does not | ||
| # allow to pass environment variables into the ctest macro expansion. | ||
| set_tests_properties( | ||
| ${testsuite} | ||
| PROPERTIES ENVIRONMENT "QT_QPA_PLATFORM=offscreen" | ||
| ) | ||
|
|
There was a problem hiding this comment.
I think this doesn't work when using the discovery variant. You should be able to set the property directly in the discovery definition, like here
There was a problem hiding this comment.
Yes It does not because test discovery is delayed work which is documented here:
https://cmake.org/cmake/help/latest/module/GoogleTest.html
The call with the environment is still in place, but does not work:
gtest_discover_tests(
mixxx-test
EXTRA_ARGS --logLevel info
WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}"
PROPERTIES ENVIRONMENT "QT_QPA_PLATFORM=offscreen"
TEST_LIST testsuite
DISCOVERY_MODE PRE_TEST
)
I suggest to revert the whole #15656 and work on a solution that works everywhere.
Better no ALAC test files than no beta builds.
There was a problem hiding this comment.
I don't understand why not setting the environment variable in Launchpad to make this work (QT_QPA_PLATFORM=offscreen)
Looking at our CI (Deb for ubuntu-24.04), this should work. Please provide access to launchpad so we can investigate it in case it does not. Launchpad has already been the reason for reverting QML changes and reverting should stop being the default way forward.
IMO, our CI should be the source of truth; If it works there, then other platform should put the right effort to make it work too.
|
The issue is not Launchpad/Ubuntu it is about RPM builds. See here: #3401 I have not fully understand the issue that requires PRE_TEST recovery, but I can Imagine that we can simply not use value-parameterized test suites to avoid such complexity. |
This is an important feature of GTest that we should definitely use to keep our test suite maintainable and debugable. I also have ongoing work, that make use of this essential functionality. |
JoergAtGithub
left a comment
JoergAtGithub
left a comment
There was a problem hiding this comment.
Discovery mode needs to be fixed before this can be merged.
|
This was the wrong approach. I will close it. |
3 participants
The comments sound like that requiring "QT_QPA_PLATFORM=offscreen" before running ctest is not always possible.
I have reverted the change to keep this environment variable as CMake property.