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

@@ -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

.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

.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

.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

.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

.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
 

.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<<EOF" >> $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 }}
+            ```

.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"]
     }
   ]
 }

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
 

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,
         )
 

scripts/requirements.txt

@@ -4,3 +4,4 @@ python-dotenv
 pyyaml
 dmgbuild; sys_platform == 'darwin'
 aqtinstall; sys_platform == 'win32' or sys_platform == 'darwin'
+colorama

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)

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

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 <gtest/gtest.h>
 

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 <fstream>
 #include <gtest/gtest.h>

src/test/shared/TestEventQueue.cpp

@@ -15,7 +15,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include "test/global/TestEventQueue.h"
+#include "test/shared/TestEventQueue.h"
 
 #include "base/Log.h"
 #include "base/SimpleEventQueueBuffer.h"

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 <http://www.gnu.org/licenses/>.
+ */
+
+#ifdef QT_GUI_LIB
+
+#include "QtCoreTest.h"
+
+std::unique_ptr<QCoreApplication> QtCoreTest::s_app;
+
+#endif

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 <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+#include <gtest/gtest.h>
+#include <qcoreapplication.h>
+
+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<QCoreApplication>(argc, argv);
+  }
+
+  static void TearDownTestSuite() {
+    s_app.reset();
+    GTEST_LOG_(INFO) << "Qt core app teardown";
+  }
+
+  static std::unique_ptr<QCoreApplication> s_app;
+};

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 <http://www.gnu.org/licenses/>.
+ */
+
+#ifdef QT_GUI_LIB
+
+#include "QtTest.h"
+
+std::unique_ptr<QApplication> QtTest::s_app;
+
+#endif

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 <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+#include <gtest/gtest.h>
+#include <qapplication.h>
+
+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<QApplication>(argc, argv);
+  }
+
+  static void TearDownTestSuite() {
+    s_app.reset();
+    GTEST_LOG_(INFO) << "Qt app teardown";
+  }
+
+  static std::unique_ptr<QApplication> s_app;
+};

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

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 <http://www.gnu.org/licenses/>.
+ */
+
 #include "MainWindow.h"
 
+#include "test/shared/gui/QtTest.h"
+
 #include <gtest/gtest.h>
 #include <memory>
 
+class MainWindowTests : public QtTest {
+public:
+  static void SetUpTestSuite() {
+    QtTest::SetUpTestSuite();
+    qRegisterMetaType<Edition>("Edition");
+  }
+
+  static std::shared_ptr<QApplication> s_app;
+};
+
 class TestMainWindow {
 public:
   class MainWindowProxy : public MainWindow {
@@ -37,7 +66,7 @@ public:
   std::shared_ptr<MainWindowProxy> 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";

src/test/unittests/gui/VersionCheckerTests.cpp

@@ -17,12 +17,16 @@
 
 #include "gui/src/VersionChecker.h"
 
+#include "test/shared/gui/QtCoreTest.h"
+
 #include <QNetworkAccessManager>
 #include <gtest/gtest.h>
 
+class VersionCheckerTests : public QtCoreTest {};
+
 class QNetworkAccessManagerMock : public QNetworkAccessManager {};
 
-TEST(VersionCheckerTests, compareVersions_major_isValid) {
+TEST_F(VersionCheckerTests, compareVersions_major_isValid) {
   auto nam = std::make_shared<QNetworkAccessManager>();
   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<QNetworkAccessManager>();
   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<QNetworkAccessManager>();
   VersionChecker versionChecker(nam);
 

src/test/unittests/main.cpp

@@ -18,8 +18,6 @@
 
 #include "arch/Arch.h"
 #include "base/Log.h"
-#include <memory>
-#include <qapplication.h>
 
 #if SYSAPI_WIN32
 #include "arch/win32/ArchMiscWindows.h"
@@ -29,9 +27,6 @@
 #include <gtest/gtest.h>
 
 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));