Skip to content

Fix Unittest on Launchpad by partially reverting #15656 #15673

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.

Sign up for GitHub

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

Closed
daschuer wants to merge 1 commit into from
Closed

Fix Unittest on Launchpad by partially reverting #15656 #15673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .github/workflows/checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,6 @@ jobs:
run: ctest --timeout 45
working-directory: build
env:
# Render analyzer waveform tests to an offscreen buffer
QT_QPA_PLATFORM: offscreen
GTEST_COLOR: 1
# Only use single thread to prevent *.gcna files from overwriting each other
CTEST_PARALLEL_LEVEL: 1
Expand Down
8 changes: 8 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2833,6 +2833,14 @@ if(BUILD_TESTING)
DISCOVERY_MODE PRE_TEST
)

# 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"
)

Comment on lines +2836 to +2843
Copy link
Copy Markdown
Member

@acolombier acolombier Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@daschuer daschuer Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@acolombier acolombier Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if(BUILD_BENCH)
# Benchmarking
add_custom_target(
Expand Down
Loading