Refactor tests and benchmarks (#4700)

.github/workflows/test.yml

@@ -7,6 +7,7 @@ on:
 
 env:
   TWITCH_PUBSUB_SERVER_IMAGE: ghcr.io/chatterino/twitch-pubsub-server-test:v1.0.6
+  QT_QPA_PLATFORM: minimal
 
 concurrency:
   group: test-${{ github.ref }}
@@ -74,7 +75,7 @@ jobs:
         if: startsWith(matrix.os, 'ubuntu')
         run: |
           cmake -DBUILD_TESTS=On -DBUILD_APP=OFF ..
-          cmake --build . --config Release
+          cmake --build .
         working-directory: build-test
         shell: bash
 
@@ -85,6 +86,6 @@ jobs:
           docker pull ${{ env.TWITCH_PUBSUB_SERVER_IMAGE }}
           docker run --network=host --detach ${{ env.TWITCH_PUBSUB_SERVER_IMAGE }}
           docker run -p 9051:80 --detach kennethreitz/httpbin
-          ./bin/chatterino-test --platform minimal || ./bin/chatterino-test --platform minimal || ./bin/chatterino-test --platform minimal
+          ./bin/chatterino-test || ./bin/chatterino-test || ./bin/chatterino-test
         working-directory: build-test
         shell: bash

CHANGELOG.md

@@ -28,6 +28,7 @@
 - Dev: Added support for compiling with `sccache`. (#4678)
 - Dev: Added `sccache` in Windows CI. (#4678)
 - Dev: Moved preprocessor Git and date definitions to executables only. (#4681)
+- Dev: Refactored tests to be able to use `ctest` and run in debug builds. (#4700)
 
 ## 2.4.4
 

CMakeLists.txt

@@ -138,6 +138,7 @@ find_package(RapidJSON REQUIRED)
 find_package(Websocketpp REQUIRED)
 
 if (BUILD_TESTS)
+    include(GoogleTest)
     # For MSVC: Prevent overriding the parent project's compiler/linker settings
     # See https://github.com/google/googletest/blob/main/googletest/README.md#visual-studio-dynamic-vs-static-runtimes
     set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)

benchmarks/src/main.cpp

@@ -18,12 +18,12 @@ int main(int argc, char **argv)
     settingsDir.setAutoRemove(false);  // we'll remove it manually
     chatterino::Settings settings(settingsDir.path());
 
-    QtConcurrent::run([&app, &settingsDir]() mutable {
+    QTimer::singleShot(0, [&]() {
         ::benchmark::RunSpecifiedBenchmarks();
 
         settingsDir.remove();
-        app.exit(0);
+        QApplication::exit(0);
     });
 
-    return app.exec();
+    return QApplication::exec();
 }

src/messages/Image.cpp

@@ -595,8 +595,8 @@ ImageExpirationPool::ImageExpirationPool()
 
 ImageExpirationPool &ImageExpirationPool::instance()
 {
-    static ImageExpirationPool instance;
+    static auto *instance = new ImageExpirationPool;
-    return instance;
+    return *instance;
 }
 
 void ImageExpirationPool::addImagePtr(ImagePtr imgPtr)

tests/CMakeLists.txt

@@ -60,10 +60,4 @@ if(CHATTERINO_TEST_USE_PUBLIC_HTTPBIN)
     target_compile_definitions(${PROJECT_NAME} PRIVATE CHATTERINO_TEST_USE_PUBLIC_HTTPBIN)
 endif()
 
-# gtest_add_tests manages to discover the tests because it looks through the source files
+gtest_discover_tests(${PROJECT_NAME})
-# HOWEVER, it fails to run, because we have some bug that causes the QApplication exit to stall when no network requests have been made.
-# ctest runs each test individually, so for now we require that testers just run the ./bin/chatterino-test binary without any filters applied
-# gtest_add_tests(
-#     TARGET ${PROJECT_NAME}
-#     SOURCES ${test_SOURCES}
-#     )

tests/src/NetworkRequest.cpp

@@ -3,17 +3,18 @@
 #include "common/NetworkManager.hpp"
 #include "common/NetworkResult.hpp"
 #include "common/Outcome.hpp"
-#include "common/QLogging.hpp"
-#include "providers/twitch/api/Helix.hpp"
 
 #include <gtest/gtest.h>
+#include <QCoreApplication>
 
 using namespace chatterino;
 
 namespace {
 
 #ifdef CHATTERINO_TEST_USE_PUBLIC_HTTPBIN
-const char *const HTTPBIN_BASE_URL = "http://httpbin.org";
+// Not using httpbin.org, since it can be really slow and cause timeouts.
+// postman-echo has the same API.
+const char *const HTTPBIN_BASE_URL = "https://postman-echo.com";
 #else
 const char *const HTTPBIN_BASE_URL = "http://127.0.0.1:9051";
 #endif
@@ -28,6 +29,46 @@ QString getDelayURL(int delay)
     return QString("%1/delay/%2").arg(HTTPBIN_BASE_URL).arg(delay);
 }
 
+class RequestWaiter
+{
+public:
+    void requestDone()
+    {
+        {
+            std::unique_lock lck(this->mutex_);
+            ASSERT_FALSE(this->requestDone_);
+            this->requestDone_ = true;
+        }
+        this->condition_.notify_one();
+    }
+
+    void waitForRequest()
+    {
+        using namespace std::chrono_literals;
+
+        while (true)
+        {
+            {
+                std::unique_lock lck(this->mutex_);
+                bool done = this->condition_.wait_for(lck, 10ms, [this] {
+                    return this->requestDone_;
+                });
+                if (done)
+                {
+                    break;
+                }
+            }
+            QCoreApplication::processEvents(QEventLoop::AllEvents);
+            QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete);
+        }
+    }
+
+private:
+    std::mutex mutex_;
+    std::condition_variable condition_;
+    bool requestDone_ = false;
+};
+
 }  // namespace
 
 TEST(NetworkRequest, Success)
@@ -39,39 +80,23 @@ TEST(NetworkRequest, Success)
     for (const auto code : codes)
     {
         auto url = getStatusURL(code);
-        std::mutex mut;
+        RequestWaiter waiter;
-        bool requestDone = false;
-        std::condition_variable requestDoneCondition;
 
         NetworkRequest(url)
-            .onSuccess([code, &mut, &requestDone, &requestDoneCondition,
+            .onSuccess(
-                        url](NetworkResult result) -> Outcome {
+                [code, &waiter, url](const NetworkResult &result) -> Outcome {
                     EXPECT_EQ(result.status(), code);
-
+                    waiter.requestDone();
-                {
-                    std::unique_lock lck(mut);
-                    requestDone = true;
-                }
-                requestDoneCondition.notify_one();
                     return Success;
                 })
-            .onError([&](NetworkResult result) {
+            .onError([&](const NetworkResult & /*result*/) {
                 // The codes should *not* throw an error
                 EXPECT_TRUE(false);
-
+                waiter.requestDone();
-                {
-                    std::unique_lock lck(mut);
-                    requestDone = true;
-                }
-                requestDoneCondition.notify_one();
             })
             .execute();
 
-        // Wait for the request to finish
+        waiter.waitForRequest();
-        std::unique_lock lck(mut);
-        requestDoneCondition.wait(lck, [&requestDone] {
-            return requestDone;
-        });
     }
 
     EXPECT_TRUE(NetworkManager::workerThread.isRunning());
@@ -86,30 +111,18 @@ TEST(NetworkRequest, FinallyCallbackOnSuccess)
     for (const auto code : codes)
     {
         auto url = getStatusURL(code);
-        std::mutex mut;
+        RequestWaiter waiter;
-        bool requestDone = false;
-        std::condition_variable requestDoneCondition;
 
         bool finallyCalled = false;
 
         NetworkRequest(url)
-            .finally(
+            .finally([&waiter, &finallyCalled] {
-                [&mut, &requestDone, &requestDoneCondition, &finallyCalled] {
                 finallyCalled = true;
-
+                waiter.requestDone();
-                    {
-                        std::unique_lock lck(mut);
-                        requestDone = true;
-                    }
-                    requestDoneCondition.notify_one();
             })
             .execute();
 
-        // Wait for the request to finish
+        waiter.waitForRequest();
-        std::unique_lock lck(mut);
-        requestDoneCondition.wait(lck, [&requestDone] {
-            return requestDone;
-        });
 
         EXPECT_TRUE(finallyCalled);
     }
@@ -127,45 +140,24 @@ TEST(NetworkRequest, Error)
     for (const auto code : codes)
     {
         auto url = getStatusURL(code);
-        std::mutex mut;
+        RequestWaiter waiter;
-        bool requestDone = false;
-        std::condition_variable requestDoneCondition;
 
         NetworkRequest(url)
-            .onSuccess([code, &mut, &requestDone, &requestDoneCondition,
+            .onSuccess(
-                        url](NetworkResult result) -> Outcome {
+                [&waiter, url](const NetworkResult & /*result*/) -> Outcome {
                     // The codes should throw an error
                     EXPECT_TRUE(false);
-
+                    waiter.requestDone();
-                {
-                    std::unique_lock lck(mut);
-                    requestDone = true;
-                }
-                requestDoneCondition.notify_one();
                     return Success;
                 })
-            .onError([code, &mut, &requestDone, &requestDoneCondition,
+            .onError([code, &waiter, url](const NetworkResult &result) {
-                      url](NetworkResult result) {
                 EXPECT_EQ(result.status(), code);
-                if (code == 402)
-                {
-                    EXPECT_EQ(result.getData(),
-                              QString("Fuck you, pay me!").toUtf8());
-                }
 
-                {
+                waiter.requestDone();
-                    std::unique_lock lck(mut);
-                    requestDone = true;
-                }
-                requestDoneCondition.notify_one();
             })
             .execute();
 
-        // Wait for the request to finish
+        waiter.waitForRequest();
-        std::unique_lock lck(mut);
-        requestDoneCondition.wait(lck, [&requestDone] {
-            return requestDone;
-        });
     }
 
     EXPECT_TRUE(NetworkManager::workerThread.isRunning());
@@ -183,30 +175,18 @@ TEST(NetworkRequest, FinallyCallbackOnError)
     for (const auto code : codes)
     {
         auto url = getStatusURL(code);
-        std::mutex mut;
+        RequestWaiter waiter;
-        bool requestDone = false;
-        std::condition_variable requestDoneCondition;
 
         bool finallyCalled = false;
 
         NetworkRequest(url)
-            .finally(
+            .finally([&waiter, &finallyCalled] {
-                [&mut, &requestDone, &requestDoneCondition, &finallyCalled] {
                 finallyCalled = true;
-
+                waiter.requestDone();
-                    {
-                        std::unique_lock lck(mut);
-                        requestDone = true;
-                    }
-                    requestDoneCondition.notify_one();
             })
             .execute();
 
-        // Wait for the request to finish
+        waiter.waitForRequest();
-        std::unique_lock lck(mut);
-        requestDoneCondition.wait(lck, [&requestDone] {
-            return requestDone;
-        });
 
         EXPECT_TRUE(finallyCalled);
     }
@@ -217,44 +197,26 @@ TEST(NetworkRequest, TimeoutTimingOut)
     EXPECT_TRUE(NetworkManager::workerThread.isRunning());
 
     auto url = getDelayURL(5);
-
+    RequestWaiter waiter;
-    std::mutex mut;
-    bool requestDone = false;
-    std::condition_variable requestDoneCondition;
 
     NetworkRequest(url)
         .timeout(1000)
-        .onSuccess([&mut, &requestDone, &requestDoneCondition,
+        .onSuccess([&waiter](const NetworkResult & /*result*/) -> Outcome {
-                    url](NetworkResult result) -> Outcome {
             // The timeout should throw an error
             EXPECT_TRUE(false);
-
+            waiter.requestDone();
-            {
-                std::unique_lock lck(mut);
-                requestDone = true;
-            }
-            requestDoneCondition.notify_one();
             return Success;
         })
-        .onError([&mut, &requestDone, &requestDoneCondition,
+        .onError([&waiter, url](const NetworkResult &result) {
-                  url](NetworkResult result) {
             qDebug() << QTime::currentTime().toString()
                      << "timeout request finish error";
             EXPECT_EQ(result.status(), NetworkResult::timedoutStatus);
 
-            {
+            waiter.requestDone();
-                std::unique_lock lck(mut);
-                requestDone = true;
-            }
-            requestDoneCondition.notify_one();
         })
         .execute();
 
-    // Wait for the request to finish
+    waiter.waitForRequest();
-    std::unique_lock lck(mut);
-    requestDoneCondition.wait(lck, [&requestDone] {
-        return requestDone;
-    });
 
     EXPECT_TRUE(NetworkManager::workerThread.isRunning());
 }
@@ -264,42 +226,24 @@ TEST(NetworkRequest, TimeoutNotTimingOut)
     EXPECT_TRUE(NetworkManager::workerThread.isRunning());
 
     auto url = getDelayURL(1);
-
+    RequestWaiter waiter;
-    std::mutex mut;
-    bool requestDone = false;
-    std::condition_variable requestDoneCondition;
 
     NetworkRequest(url)
-        .timeout(2000)
+        .timeout(3000)
-        .onSuccess([&mut, &requestDone, &requestDoneCondition,
+        .onSuccess([&waiter, url](const NetworkResult &result) -> Outcome {
-                    url](NetworkResult result) -> Outcome {
             EXPECT_EQ(result.status(), 200);
 
-            {
+            waiter.requestDone();
-                std::unique_lock lck(mut);
-                requestDone = true;
-            }
-            requestDoneCondition.notify_one();
             return Success;
         })
-        .onError([&mut, &requestDone, &requestDoneCondition,
+        .onError([&waiter, url](const NetworkResult & /*result*/) {
-                  url](NetworkResult result) {
             // The timeout should *not* throw an error
             EXPECT_TRUE(false);
-
+            waiter.requestDone();
-            {
-                std::unique_lock lck(mut);
-                requestDone = true;
-            }
-            requestDoneCondition.notify_one();
         })
         .execute();
 
-    // Wait for the request to finish
+    waiter.waitForRequest();
-    std::unique_lock lck(mut);
-    requestDoneCondition.wait(lck, [&requestDone] {
-        return requestDone;
-    });
 
     EXPECT_TRUE(NetworkManager::workerThread.isRunning());
 }
@@ -310,39 +254,28 @@ TEST(NetworkRequest, FinallyCallbackOnTimeout)
 
     auto url = getDelayURL(5);
 
-    std::mutex mut;
+    RequestWaiter waiter;
-    bool requestDone = false;
-    std::condition_variable requestDoneCondition;
     bool finallyCalled = false;
     bool onSuccessCalled = false;
     bool onErrorCalled = false;
 
     NetworkRequest(url)
         .timeout(1000)
-        .onSuccess([&](NetworkResult result) -> Outcome {
+        .onSuccess([&](const NetworkResult & /*result*/) -> Outcome {
             onSuccessCalled = true;
             return Success;
         })
-        .onError([&](NetworkResult result) {
+        .onError([&](const NetworkResult &result) {
             onErrorCalled = true;
             EXPECT_EQ(result.status(), NetworkResult::timedoutStatus);
         })
         .finally([&] {
             finallyCalled = true;
-
+            waiter.requestDone();
-            {
-                std::unique_lock lck(mut);
-                requestDone = true;
-            }
-            requestDoneCondition.notify_one();
         })
         .execute();
 
-    // Wait for the request to finish
+    waiter.waitForRequest();
-    std::unique_lock lck(mut);
-    requestDoneCondition.wait(lck, [&requestDone] {
-        return requestDone;
-    });
 
     EXPECT_TRUE(finallyCalled);
     EXPECT_TRUE(onErrorCalled);

tests/src/main.cpp

@@ -32,16 +32,16 @@ int main(int argc, char **argv)
     qDebug() << "Settings directory:" << settingsDir.path();
     chatterino::Settings settings(settingsDir.path());
 
-    QtConcurrent::run([&app, &settingsDir]() mutable {
+    QTimer::singleShot(0, [&]() {
         auto res = RUN_ALL_TESTS();
 
         chatterino::NetworkManager::deinit();
 
         settingsDir.remove();
-        app.exit(res);
+        QApplication::exit(res);
     });
 
-    return app.exec();
+    return QApplication::exec();
 #else
     return RUN_ALL_TESTS();
 #endif