From 47849db4d9af6e1a7557dc6b98737f95d87aa743 Mon Sep 17 00:00:00 2001 From: Nick Bolton Date: Wed, 17 Jul 2024 09:22:46 +0100 Subject: [PATCH] Run Valgrind on unit tests in CI to detect memory leaks (#7401) * Move QApplication out of main to reduce memory impact when running individual tests * Add --valgrind arg and colorize output when command returns non-zero exit code * Fixed: colorama not always available * Test multiple Qt tests * Fixed: Windows Qt test failing due to missing QCoreApplication * Simplify fake args for Qt * Use --ci-env arg * Create Valgrind analysis workflow * Rename vars for fake args * Parse and output valgrind summary * Add build mode to comment * Use GITHUB_OUTPUT to output summary * Merge valgrind comment * Improve comment * Use `tee` instead of `--log-file` to also print stdout * Improve comment about debug and release * Simplify output writing in parse step * Improve step name * Correct comment about summaries * Remove commented out code * Better var name * Missing copyright * Rename global to shared * Remove space * Revert change to ConfigTests.cpp --- .github/docker/archlinux/Dockerfile | 2 +- .github/docker/debian/Dockerfile | 2 +- .github/docker/fedora/Dockerfile | 2 +- .github/docker/opensuse/Dockerfile | 2 +- .github/workflows/codeql-analysis.yml | 2 +- .github/workflows/sonarcloud-analysis.yml | 2 +- .github/workflows/valgrind-analysis.yml | 55 ++++++++++++++++++- .vscode/tasks.json | 26 +++++++++ ChangeLog | 1 + scripts/lib/cmd_utils.py | 13 ++++- scripts/requirements.txt | 1 + scripts/tests.py | 13 +++++ src/test/integtests/CMakeLists.txt | 8 +-- src/test/integtests/ipc/IpcTests.cpp | 2 +- src/test/integtests/net/NetworkTests.cpp | 2 +- .../{global => shared}/TestEventQueue.cpp | 2 +- src/test/{global => shared}/TestEventQueue.h | 0 src/test/shared/gui/QtCoreTest.cpp | 24 ++++++++ src/test/shared/gui/QtCoreTest.h | 38 +++++++++++++ src/test/shared/gui/QtTest.cpp | 24 ++++++++ src/test/shared/gui/QtTest.h | 38 +++++++++++++ src/test/unittests/CMakeLists.txt | 8 +-- src/test/unittests/gui/MainWindowTests.cpp | 33 ++++++++++- .../unittests/gui/VersionCheckerTests.cpp | 10 +++- src/test/unittests/main.cpp | 5 -- 25 files changed, 285 insertions(+), 30 deletions(-) rename src/test/{global => shared}/TestEventQueue.cpp (97%) rename src/test/{global => shared}/TestEventQueue.h (100%) create mode 100644 src/test/shared/gui/QtCoreTest.cpp create mode 100644 src/test/shared/gui/QtCoreTest.h create mode 100644 src/test/shared/gui/QtTest.cpp create mode 100644 src/test/shared/gui/QtTest.h diff --git a/.github/docker/archlinux/Dockerfile b/.github/docker/archlinux/Dockerfile index 172d2f182..1431dcbec 100644 --- a/.github/docker/archlinux/Dockerfile +++ b/.github/docker/archlinux/Dockerfile @@ -13,5 +13,5 @@ RUN useradd -m build WORKDIR /app RUN --mount=type=bind,target=/app,rw \ - ./scripts/install_deps.py && \ + ./scripts/install_deps.py --ci-env && \ pacman -Scc --noconfirm diff --git a/.github/docker/debian/Dockerfile b/.github/docker/debian/Dockerfile index 28a24804d..ea9322cbd 100644 --- a/.github/docker/debian/Dockerfile +++ b/.github/docker/debian/Dockerfile @@ -13,5 +13,5 @@ RUN apt update && \ WORKDIR /app RUN --mount=type=bind,target=/app,rw \ - ./scripts/install_deps.py && \ + ./scripts/install_deps.py --ci-env && \ apt clean diff --git a/.github/docker/fedora/Dockerfile b/.github/docker/fedora/Dockerfile index 819fd01ce..a2c390c0e 100644 --- a/.github/docker/fedora/Dockerfile +++ b/.github/docker/fedora/Dockerfile @@ -12,5 +12,5 @@ RUN dnf upgrade -y && \ WORKDIR /app RUN --mount=type=bind,target=/app,rw \ - ./scripts/install_deps.py && \ + ./scripts/install_deps.py --ci-env && \ dnf clean all diff --git a/.github/docker/opensuse/Dockerfile b/.github/docker/opensuse/Dockerfile index c075a1e12..4236521f1 100644 --- a/.github/docker/opensuse/Dockerfile +++ b/.github/docker/opensuse/Dockerfile @@ -13,5 +13,5 @@ RUN zypper refresh && \ WORKDIR /app RUN --mount=type=bind,target=/app,rw \ - ./scripts/install_deps.py && \ + ./scripts/install_deps.py --ci-env && \ zypper clean --all diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index 92c3490ee..215dae0df 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -39,7 +39,7 @@ jobs: run: git config --global --add safe.directory $GITHUB_WORKSPACE - name: Install dependencies - run: ./scripts/install_deps.py + run: ./scripts/install_deps.py --ci-env - name: Initialize CodeQL uses: github/codeql-action/init@v3 diff --git a/.github/workflows/sonarcloud-analysis.yml b/.github/workflows/sonarcloud-analysis.yml index f98b6dbd1..61af45eff 100644 --- a/.github/workflows/sonarcloud-analysis.yml +++ b/.github/workflows/sonarcloud-analysis.yml @@ -39,7 +39,7 @@ jobs: - name: Install dependencies run: | - ./scripts/install_deps.py && + ./scripts/install_deps.py --ci-env && apt install curl unzip -y && pip install gcovr diff --git a/.github/workflows/valgrind-analysis.yml b/.github/workflows/valgrind-analysis.yml index 010294fa7..d8c28027c 100644 --- a/.github/workflows/valgrind-analysis.yml +++ b/.github/workflows/valgrind-analysis.yml @@ -20,5 +20,56 @@ jobs: timeout-minutes: 5 steps: - - name: Stub - run: echo stub + - name: Checkout + uses: actions/checkout@v4 + with: + submodules: "recursive" + + - name: Config Git safe dir + run: git config --global --add safe.directory $GITHUB_WORKSPACE + + - name: Install dependencies + run: | + ./scripts/install_deps.py --ci-env && + apt install valgrind -y + + - name: Configure + run: cmake -B build --preset=linux-release + + - name: Build + run: cmake --build build -j8 + + - name: Run Valgrind on unit tests + env: + QT_QPA_PLATFORM: offscreen + run: | + valgrind \ + --leak-check=full \ + --show-leak-kinds=all \ + --track-origins=yes \ + --verbose \ + ./build/bin/unittests \ + 2>&1 | tee valgrind.log + + - name: Parse summary + id: parse + run: | + echo "summary<> $GITHUB_OUTPUT + echo "$(grep -A 2 "HEAP SUMMARY:" valgrind.log)" >> $GITHUB_OUTPUT + echo >> $GITHUB_OUTPUT + echo "$(awk '/LEAK SUMMARY/,/ERROR SUMMARY/' valgrind.log)" >> $GITHUB_OUTPUT + echo "EOF" >> $GITHUB_OUTPUT + + - name: Append to PR comment + uses: marocchino/sticky-pull-request-comment@v2 + env: + URL: https://github.com/symless/synergy-core/actions/workflows/valgrind-analysis.yml + with: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + recreate: true + message: | + ## Valgrind summary + See [workflow output](${{ env.URL }}) for full `valgrind` output. + ``` + ${{ steps.parse.outputs.summary }} + ``` diff --git a/.vscode/tasks.json b/.vscode/tasks.json index e5107c9c3..9f24b1ed0 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -69,6 +69,32 @@ "command": "python", "args": ["./scripts/tests.py", "--integ-tests", "--ignore-return-code"], "dependsOn": ["build"] + }, + { + "label": "unittests (current, valgrind)", + "type": "shell", + "command": "python", + "args": [ + "./scripts/tests.py", + "--unit-tests", + "--ignore-return-code", + "--filter-file=${file}", + "--valgrind" + ], + "dependsOn": ["build"] + }, + { + "label": "integtests (current, valgrind)", + "type": "shell", + "command": "python", + "args": [ + "./scripts/tests.py", + "--integ-tests", + "--ignore-return-code", + "--filter-file=${file}", + "--valgrind" + ], + "dependsOn": ["build"] } ] } diff --git a/ChangeLog b/ChangeLog index 559d4361e..0de307bd3 100644 --- a/ChangeLog +++ b/ChangeLog @@ -53,6 +53,7 @@ Enhancements: - #7384 Run `install_deps.py` script when building containers weekly - #7389 Correct Qt macOS target and drop `Core5Compat` lib - #7383 Solve SonarCloud security hotspots and bugs +- #7401 Run Valgrind on unit tests in CI to detect memory leaks # 1.14.6 diff --git a/scripts/lib/cmd_utils.py b/scripts/lib/cmd_utils.py index 0a4e44cc8..1246d2468 100644 --- a/scripts/lib/cmd_utils.py +++ b/scripts/lib/cmd_utils.py @@ -2,6 +2,17 @@ import subprocess import sys import lib.env as env +try: + import colorama # type: ignore + from colorama import Fore # type: ignore + + colorama.init() +except ImportError: + + class Fore: + RESET = "" + YELLOW = "" + def has_command(command): platform = sys.platform @@ -120,7 +131,7 @@ def run( if result.returncode != 0: print( - f"Command exited with code {result.returncode}: {command_str}", + f"{Fore.YELLOW}Command exited with code {result.returncode}:{Fore.RESET} {command_str}", file=sys.stderr, ) diff --git a/scripts/requirements.txt b/scripts/requirements.txt index 6b5becaae..fd3917844 100644 --- a/scripts/requirements.txt +++ b/scripts/requirements.txt @@ -4,3 +4,4 @@ python-dotenv pyyaml dmgbuild; sys_platform == 'darwin' aqtinstall; sys_platform == 'win32' or sys_platform == 'darwin' +colorama diff --git a/scripts/tests.py b/scripts/tests.py index 123d173aa..181ab50d1 100755 --- a/scripts/tests.py +++ b/scripts/tests.py @@ -2,9 +2,13 @@ import argparse, os, sys import lib.cmd_utils as cmd_utils +import lib.env as env def main(): + # important: load venv before loading modules that install deps. + env.ensure_in_venv(__file__) + parser = argparse.ArgumentParser() parser.add_argument("--unit-tests", action="store_true") parser.add_argument("--integ-tests", action="store_true") @@ -18,9 +22,15 @@ def main(): action="store_true", help="Ignore the return code of the test command", ) + parser.add_argument( + "--valgrind", + action="store_true", + help="Run the test command with valgrind", + ) args = parser.parse_args() binary = get_binary_path(args) + if args.filter_file: file_base = os.path.basename(args.filter_file) without_ext = os.path.splitext(file_base)[0] @@ -28,6 +38,9 @@ def main(): else: command = [binary] + if args.valgrind: + command = ["valgrind"] + command + result = cmd_utils.run(command, print_cmd=True, check=False) if not args.ignore_return_code: sys.exit(result.returncode) diff --git a/src/test/integtests/CMakeLists.txt b/src/test/integtests/CMakeLists.txt index b1a401c14..58c256a04 100644 --- a/src/test/integtests/CMakeLists.txt +++ b/src/test/integtests/CMakeLists.txt @@ -37,11 +37,11 @@ endif() list(APPEND sources ${platform_sources}) list(APPEND headers ${platform_headers}) -file(GLOB_RECURSE global_headers "../../test/global/*.h") -file(GLOB_RECURSE global_sources "../../test/global/*.cpp") +file(GLOB_RECURSE shared_headers "../../test/shared/*.h") +file(GLOB_RECURSE shared_sources "../../test/shared/*.cpp") -list(APPEND headers ${global_headers}) -list(APPEND sources ${global_sources}) +list(APPEND headers ${shared_headers}) +list(APPEND sources ${shared_sources}) file(GLOB_RECURSE mock_headers "../../test/mock/*.h") file(GLOB_RECURSE mock_sources "../../test/mock/*.cpp") diff --git a/src/test/integtests/ipc/IpcTests.cpp b/src/test/integtests/ipc/IpcTests.cpp index fa585c1d9..ad42732a6 100644 --- a/src/test/integtests/ipc/IpcTests.cpp +++ b/src/test/integtests/ipc/IpcTests.cpp @@ -35,7 +35,7 @@ #include "ipc/IpcServerProxy.h" #include "mt/Thread.h" #include "net/SocketMultiplexer.h" -#include "test/global/TestEventQueue.h" +#include "test/shared/TestEventQueue.h" #include diff --git a/src/test/integtests/net/NetworkTests.cpp b/src/test/integtests/net/NetworkTests.cpp index d7438d310..8f5d39426 100644 --- a/src/test/integtests/net/NetworkTests.cpp +++ b/src/test/integtests/net/NetworkTests.cpp @@ -34,11 +34,11 @@ #include "server/Server.h" #include "synergy/FileChunk.h" #include "synergy/StreamChunker.h" -#include "test/global/TestEventQueue.h" #include "test/mock/server/MockConfig.h" #include "test/mock/server/MockInputFilter.h" #include "test/mock/server/MockPrimaryClient.h" #include "test/mock/synergy/MockScreen.h" +#include "test/shared/TestEventQueue.h" #include #include diff --git a/src/test/global/TestEventQueue.cpp b/src/test/shared/TestEventQueue.cpp similarity index 97% rename from src/test/global/TestEventQueue.cpp rename to src/test/shared/TestEventQueue.cpp index 96c5a6c8f..454b745c5 100644 --- a/src/test/global/TestEventQueue.cpp +++ b/src/test/shared/TestEventQueue.cpp @@ -15,7 +15,7 @@ * along with this program. If not, see . */ -#include "test/global/TestEventQueue.h" +#include "test/shared/TestEventQueue.h" #include "base/Log.h" #include "base/SimpleEventQueueBuffer.h" diff --git a/src/test/global/TestEventQueue.h b/src/test/shared/TestEventQueue.h similarity index 100% rename from src/test/global/TestEventQueue.h rename to src/test/shared/TestEventQueue.h diff --git a/src/test/shared/gui/QtCoreTest.cpp b/src/test/shared/gui/QtCoreTest.cpp new file mode 100644 index 000000000..8331e4694 --- /dev/null +++ b/src/test/shared/gui/QtCoreTest.cpp @@ -0,0 +1,24 @@ +/* + * synergy -- mouse and keyboard sharing utility + * Copyright (C) 2024 Symless Ltd. + * + * This package is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * found in the file LICENSE that should have accompanied this file. + * + * This package is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#ifdef QT_GUI_LIB + +#include "QtCoreTest.h" + +std::unique_ptr QtCoreTest::s_app; + +#endif diff --git a/src/test/shared/gui/QtCoreTest.h b/src/test/shared/gui/QtCoreTest.h new file mode 100644 index 000000000..1f18d6be2 --- /dev/null +++ b/src/test/shared/gui/QtCoreTest.h @@ -0,0 +1,38 @@ +/* + * synergy -- mouse and keyboard sharing utility + * Copyright (C) 2024 Symless Ltd. + * + * This package is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * found in the file LICENSE that should have accompanied this file. + * + * This package is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#pragma once + +#include +#include + +class QtCoreTest : public ::testing::Test { +public: + static void SetUpTestSuite() { + GTEST_LOG_(INFO) << "Qt core app setup"; + char **argv = nullptr; + int argc = 0; + s_app = std::make_unique(argc, argv); + } + + static void TearDownTestSuite() { + s_app.reset(); + GTEST_LOG_(INFO) << "Qt core app teardown"; + } + + static std::unique_ptr s_app; +}; diff --git a/src/test/shared/gui/QtTest.cpp b/src/test/shared/gui/QtTest.cpp new file mode 100644 index 000000000..984cb9416 --- /dev/null +++ b/src/test/shared/gui/QtTest.cpp @@ -0,0 +1,24 @@ +/* + * synergy -- mouse and keyboard sharing utility + * Copyright (C) 2024 Symless Ltd. + * + * This package is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * found in the file LICENSE that should have accompanied this file. + * + * This package is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#ifdef QT_GUI_LIB + +#include "QtTest.h" + +std::unique_ptr QtTest::s_app; + +#endif diff --git a/src/test/shared/gui/QtTest.h b/src/test/shared/gui/QtTest.h new file mode 100644 index 000000000..b8906dc1d --- /dev/null +++ b/src/test/shared/gui/QtTest.h @@ -0,0 +1,38 @@ +/* + * synergy -- mouse and keyboard sharing utility + * Copyright (C) 2024 Symless Ltd. + * + * This package is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * found in the file LICENSE that should have accompanied this file. + * + * This package is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#pragma once + +#include +#include + +class QtTest : public ::testing::Test { +public: + static void SetUpTestSuite() { + GTEST_LOG_(INFO) << "Qt app setup"; + char **argv = nullptr; + int argc = 0; + s_app = std::make_unique(argc, argv); + } + + static void TearDownTestSuite() { + s_app.reset(); + GTEST_LOG_(INFO) << "Qt app teardown"; + } + + static std::unique_ptr s_app; +}; diff --git a/src/test/unittests/CMakeLists.txt b/src/test/unittests/CMakeLists.txt index ee14f63aa..ef9117b42 100644 --- a/src/test/unittests/CMakeLists.txt +++ b/src/test/unittests/CMakeLists.txt @@ -23,11 +23,11 @@ file(GLOB_RECURSE remove_platform "platform/*") list(REMOVE_ITEM headers ${remove_platform}) list(REMOVE_ITEM sources ${remove_platform}) -file(GLOB_RECURSE global_headers "../../test/global/*.h") -file(GLOB_RECURSE global_sources "../../test/global/*.cpp") +file(GLOB_RECURSE shared_headers "../../test/shared/*.h") +file(GLOB_RECURSE shared_sources "../../test/shared/*.cpp") -list(APPEND headers ${global_headers}) -list(APPEND sources ${global_sources}) +list(APPEND headers ${shared_headers}) +list(APPEND sources ${shared_sources}) file(GLOB_RECURSE mock_headers "../../test/mock/*.h") file(GLOB_RECURSE mock_sources "../../test/mock/*.cpp") diff --git a/src/test/unittests/gui/MainWindowTests.cpp b/src/test/unittests/gui/MainWindowTests.cpp index 2c1fdfb8a..51c9d4b5c 100644 --- a/src/test/unittests/gui/MainWindowTests.cpp +++ b/src/test/unittests/gui/MainWindowTests.cpp @@ -1,8 +1,37 @@ +/* + * synergy -- mouse and keyboard sharing utility + * Copyright (C) 2024 Symless Ltd. + * + * This package is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * found in the file LICENSE that should have accompanied this file. + * + * This package is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + #include "MainWindow.h" +#include "test/shared/gui/QtTest.h" + #include #include +class MainWindowTests : public QtTest { +public: + static void SetUpTestSuite() { + QtTest::SetUpTestSuite(); + qRegisterMetaType("Edition"); + } + + static std::shared_ptr s_app; +}; + class TestMainWindow { public: class MainWindowProxy : public MainWindow { @@ -37,7 +66,7 @@ public: std::shared_ptr m_mainWindow; }; -TEST(MainWindowTests, checkSecureSocket_noMatch_expectFalse) { +TEST_F(MainWindowTests, checkSecureSocket_noMatch_expectFalse) { TestMainWindow testMainWindow; bool result = testMainWindow.m_mainWindow->_checkSecureSocket("test"); @@ -45,7 +74,7 @@ TEST(MainWindowTests, checkSecureSocket_noMatch_expectFalse) { EXPECT_FALSE(result); } -TEST(MainWindowTests, checkSecureSocket_match_expectTrue) { +TEST_F(MainWindowTests, checkSecureSocket_match_expectTrue) { TestMainWindow testMainWindow; const char *test = "network encryption protocol: test"; diff --git a/src/test/unittests/gui/VersionCheckerTests.cpp b/src/test/unittests/gui/VersionCheckerTests.cpp index 0ca23767d..292f1e7b7 100644 --- a/src/test/unittests/gui/VersionCheckerTests.cpp +++ b/src/test/unittests/gui/VersionCheckerTests.cpp @@ -17,12 +17,16 @@ #include "gui/src/VersionChecker.h" +#include "test/shared/gui/QtCoreTest.h" + #include #include +class VersionCheckerTests : public QtCoreTest {}; + class QNetworkAccessManagerMock : public QNetworkAccessManager {}; -TEST(VersionCheckerTests, compareVersions_major_isValid) { +TEST_F(VersionCheckerTests, compareVersions_major_isValid) { auto nam = std::make_shared(); VersionChecker versionChecker(nam); @@ -31,7 +35,7 @@ TEST(VersionCheckerTests, compareVersions_major_isValid) { EXPECT_EQ(versionChecker.compareVersions("1.0.0", "1.0.0"), 0); } -TEST(VersionCheckerTests, compareVersions_minor_isValid) { +TEST_F(VersionCheckerTests, compareVersions_minor_isValid) { auto nam = std::make_shared(); VersionChecker versionChecker(nam); @@ -40,7 +44,7 @@ TEST(VersionCheckerTests, compareVersions_minor_isValid) { EXPECT_EQ(versionChecker.compareVersions("1.1.0", "1.1.0"), 0); } -TEST(VersionCheckerTests, compareVersions_patch_isValid) { +TEST_F(VersionCheckerTests, compareVersions_patch_isValid) { auto nam = std::make_shared(); VersionChecker versionChecker(nam); diff --git a/src/test/unittests/main.cpp b/src/test/unittests/main.cpp index 06330a77e..64c88cbb3 100644 --- a/src/test/unittests/main.cpp +++ b/src/test/unittests/main.cpp @@ -18,8 +18,6 @@ #include "arch/Arch.h" #include "base/Log.h" -#include -#include #if SYSAPI_WIN32 #include "arch/win32/ArchMiscWindows.h" @@ -29,9 +27,6 @@ #include int main(int argc, char **argv) { - // required to solve the issue where qt objects need access to a qt app. - QApplication app(argc, argv); - #if SYSAPI_WIN32 // HACK: shouldn't be needed, but logging fails without this. ArchMiscWindows::setInstanceWin32(GetModuleHandle(NULL));