Add Network tests (#2304)

Also changes the way timeouts happen, since right now if a timeout was met (which it mostly wasn't), it would run the error callback twice causing potentially undefined behaviour

CMakeLists.txt

@@ -4,15 +4,33 @@ project(chatterino)
 
 include_directories(src)
 
+add_subdirectory(lib/settings)
+
 set(chatterino_SOURCES
+    src/common/NetworkRequest.cpp
+    src/common/NetworkResult.cpp
+    src/common/NetworkPrivate.cpp
+    src/common/NetworkManager.cpp
+    src/common/QLogging.cpp
+    src/common/Modes.cpp
+    src/common/ChatterinoSetting.cpp
+
+    src/util/DebugCount.cpp
+
+    src/singletons/Paths.cpp
+
+    src/BaseSettings.cpp
+
     src/common/UsernameSet.cpp
     src/controllers/highlights/HighlightPhrase.cpp
     )
 
 find_package(Qt5 5.9.0 REQUIRED COMPONENTS
-    Core Widgets
+    Core Widgets Network Concurrent
     )
 
+set(CMAKE_AUTOMOC ON)
+
 # set(CMAKE_AUTOMOC ON)
 
 if (BUILD_TESTS)
@@ -24,14 +42,20 @@ if (BUILD_TESTS)
         ${chatterino_SOURCES}
 
         tests/src/main.cpp
+        tests/src/NetworkRequest.cpp
         tests/src/UsernameSet.cpp
         tests/src/HighlightPhrase.cpp
         )
 
-    target_link_libraries(chatterino-test Qt5::Core Qt5::Widgets)
+    target_compile_definitions(chatterino-test PRIVATE CHATTERINO_GIT_HASH="test" AB_CUSTOM_SETTINGS)
+
+    target_link_libraries(chatterino-test Qt5::Core Qt5::Widgets Qt5::Network Qt5::Concurrent)
 
     target_link_libraries(chatterino-test gtest gtest_main)
 
+    target_link_libraries(chatterino-test PajladaSettings)
+    target_include_directories(chatterino-test PUBLIC PajladaSettings)
+
     set(BUILD_TESTS OFF)
     add_subdirectory(${CMAKE_CURRENT_LIST_DIR}/lib/serialize PajladaSerialize)
 
@@ -40,7 +64,7 @@ if (BUILD_TESTS)
     set_property(TARGET chatterino-test PROPERTY CXX_STANDARD 17)
     set_property(TARGET chatterino-test PROPERTY CXX_STANDARD_REQUIRED ON)
 
-    gtest_discover_tests(chatterino-test)
+    # gtest_discover_tests(chatterino-test)
 else()
     message(FATAL_ERROR "This cmake file is only intended for tests right now. Use qmake to build chatterino2")
 endif()

lib/settings

@@ -1 +1 @@
-Subproject commit 6de3a27784745959ab261df219880cddc9263ff9
+Subproject commit cc2fb43ad515e8ff7349e2eb4ddeb3fb96337b1a

src/common/Common.hpp

@@ -29,6 +29,14 @@ const Qt::KeyboardModifiers showAddSplitRegions =
     Qt::ControlModifier | Qt::AltModifier;
 const Qt::KeyboardModifiers showResizeHandlesModifiers = Qt::ControlModifier;
 
+#ifndef ATTR_UNUSED
+#    ifdef Q_OS_WIN
+#        define ATTR_UNUSED
+#    else
+#        define ATTR_UNUSED __attribute__((unused))
+#    endif
+#endif
+
 static const char *ANONYMOUS_USERNAME_LABEL ATTR_UNUSED = " - anonymous - ";
 
 template <typename T>

src/common/NetworkPrivate.cpp

@@ -10,23 +10,20 @@
 
 #include <QCryptographicHash>
 #include <QFile>
+#include <QNetworkReply>
 #include <QtConcurrent>
 #include "common/QLogging.hpp"
 
 namespace chatterino {
 
 NetworkData::NetworkData()
-    : timer_(new QTimer())
+    : lifetimeManager_(new QObject)
-    , lifetimeManager_(new QObject)
 {
-    timer_->setSingleShot(true);
-
     DebugCount::increase("NetworkData");
 }
 
 NetworkData::~NetworkData()
 {
-    this->timer_->deleteLater();
     this->lifetimeManager_->deleteLater();
 
     DebugCount::decrease("NetworkData");
@@ -84,13 +81,14 @@ void loadUncached(const std::shared_ptr<NetworkData> &data)
 
     worker->moveToThread(&NetworkManager::workerThread);
 
+    auto onUrlRequested = [data, worker]() mutable {
         if (data->hasTimeout_)
         {
+            data->timer_ = new QTimer();
             data->timer_->setSingleShot(true);
-        data->timer_->start();
+            data->timer_->start(data->timeoutMS_);
         }
 
-    auto onUrlRequested = [data, worker]() mutable {
         auto reply = [&]() -> QNetworkReply * {
             switch (data->requestType_)
             {
@@ -128,7 +126,7 @@ void loadUncached(const std::shared_ptr<NetworkData> &data)
             return;
         }
 
-        if (data->timer_->isActive())
+        if (data->timer_ != nullptr && data->timer_->isActive())
         {
             QObject::connect(
                 data->timer_, &QTimer::timeout, worker, [reply, data]() {
@@ -158,11 +156,18 @@ void loadUncached(const std::shared_ptr<NetworkData> &data)
             // TODO(pajlada): A reply was received, kill the timeout timer
             if (reply->error() != QNetworkReply::NetworkError::NoError)
             {
+                if (reply->error() ==
+                    QNetworkReply::NetworkError::OperationCanceledError)
+                {
+                    //operation cancelled, most likely timed out
+                    return;
+                }
                 if (data->onError_)
                 {
-                    auto error = reply->error();
+                    auto status = reply->attribute(
-                    postToThread([data, error] {
+                        QNetworkRequest::HttpStatusCodeAttribute);
-                        data->onError_(NetworkResult({}, error));
+                    postToThread([data, code = status.toInt()] {
+                        data->onError_(NetworkResult({}, code));
                     });
                 }
                 return;
@@ -193,6 +198,12 @@ void loadUncached(const std::shared_ptr<NetworkData> &data)
             reply->deleteLater();
         };
 
+        if (data->timer_ != nullptr)
+        {
+            QObject::connect(reply, &QNetworkReply::finished, data->timer_,
+                             &QObject::deleteLater);
+        }
+
         QObject::connect(
             reply, &QNetworkReply::finished, worker,
             [data, handleReply, worker]() mutable {

src/common/NetworkPrivate.hpp

@@ -5,7 +5,9 @@
 
 #include <QHttpMultiPart>
 #include <QNetworkRequest>
+#include <QTimer>
 #include <functional>
+#include <memory>
 
 class QNetworkReply;
 
@@ -54,7 +56,8 @@ struct NetworkData {
     // to enable the timer, the "setTimeout" function needs to be called before
     // execute is called
     bool hasTimeout_{};
-    QTimer *timer_;
+    int timeoutMS_{};
+    QTimer *timer_ = nullptr;
     QObject *lifetimeManager_;
 
     QString getHash();

src/common/NetworkRequest.cpp

@@ -117,7 +117,7 @@ NetworkRequest NetworkRequest::headerList(const QStringList &headers) &&
 NetworkRequest NetworkRequest::timeout(int ms) &&
 {
     this->data->hasTimeout_ = true;
-    this->data->timer_->setInterval(ms);
+    this->data->timeoutMS_ = ms;
     return std::move(*this);
 }
 

src/common/NetworkResult.hpp

@@ -1,6 +1,7 @@
 #pragma once
 
 #include <rapidjson/document.h>
+#include <QJsonArray>
 #include <QJsonObject>
 
 namespace chatterino {

src/controllers/filters/FilterRecord.hpp

@@ -8,6 +8,7 @@
 
 #include <QRegularExpression>
 #include <QString>
+#include <QUuid>
 #include <pajlada/serialize.hpp>
 
 #include <memory>

src/controllers/filters/parser/Tokenizer.hpp

@@ -2,6 +2,10 @@
 
 #include "controllers/filters/parser/Types.hpp"
 
+#include <QMap>
+#include <QRegularExpression>
+#include <QString>
+
 namespace filterparser {
 
 static const QMap<QString, QString> validIdentifiersMap = {

src/providers/twitch/TwitchCommon.hpp

@@ -5,6 +5,14 @@
 
 namespace chatterino {
 
+#ifndef ATTR_UNUSED
+#    ifdef Q_OS_WIN
+#        define ATTR_UNUSED
+#    else
+#        define ATTR_UNUSED __attribute__((unused))
+#    endif
+#endif
+
 static const char *ANONYMOUS_USERNAME ATTR_UNUSED = "justinfan64537";
 
 inline QByteArray getDefaultClientID()

src/providers/twitch/api/Helix.hpp

@@ -2,6 +2,7 @@
 
 #include "common/NetworkRequest.hpp"
 
+#include <QJsonArray>
 #include <QString>
 #include <QStringList>
 #include <QUrl>

src/util/QObjectRef.hpp

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <QApplication>
 #include <QObject>
 #include <type_traits>
 

tests/.clang-format

@@ -4,6 +4,7 @@ AccessModifierOffset: -4
 AlignEscapedNewlinesLeft: true
 AllowShortFunctionsOnASingleLine: false
 AllowShortIfStatementsOnASingleLine: false
+AllowShortLambdasOnASingleLine: Empty
 AllowShortLoopsOnASingleLine: false
 AlwaysBreakAfterDefinitionReturnType: false
 AlwaysBreakBeforeMultilineStrings: false

tests/src/NetworkRequest.cpp

@@ -0,0 +1,211 @@
+#include "common/NetworkRequest.hpp"
+#include "common/NetworkManager.hpp"
+#include "common/NetworkResult.hpp"
+#include "common/Outcome.hpp"
+#include "providers/twitch/api/Helix.hpp"
+
+#include "common/Outcome.hpp"
+#include "common/QLogging.hpp"
+
+#include <gtest/gtest.h>
+
+using namespace chatterino;
+
+namespace {
+
+static QString getStatusURL(int code)
+{
+    return QString("http://httpbin.org/status/%1").arg(code);
+}
+
+}  // namespace
+
+TEST(NetworkRequest, Success)
+{
+    const std::vector<int> codes{200, 201, 202, 203, 204, 205, 206};
+
+    EXPECT_TRUE(NetworkManager::workerThread.isRunning());
+
+    for (const auto code : codes)
+    {
+        auto url = getStatusURL(code);
+        std::mutex mut;
+        bool requestDone = false;
+        std::condition_variable requestDoneCondition;
+
+        NetworkRequest(url)
+            .onSuccess([code, &mut, &requestDone, &requestDoneCondition,
+                        url](NetworkResult result) -> Outcome {
+                EXPECT_EQ(result.status(), code);
+
+                {
+                    std::unique_lock lck(mut);
+                    requestDone = true;
+                }
+                requestDoneCondition.notify_one();
+                return Success;
+            })
+            .onError([&](NetworkResult result) {
+                // The codes should *not* throw an error
+                EXPECT_TRUE(false);
+
+                {
+                    std::unique_lock lck(mut);
+                    requestDone = true;
+                }
+                requestDoneCondition.notify_one();
+            })
+            .execute();
+
+        // Wait for the request to finish
+        std::unique_lock lck(mut);
+        requestDoneCondition.wait(lck, [&requestDone] {
+            return requestDone;
+        });
+    }
+
+    EXPECT_TRUE(NetworkManager::workerThread.isRunning());
+}
+
+TEST(NetworkRequest, Error)
+{
+    const std::vector<int> codes{
+        400, 401, 402, 403, 404, 405, 406, 407, 408, 409, 410,
+        411, 412, 413, 414, 418, 500, 501, 502, 503, 504,
+    };
+
+    EXPECT_TRUE(NetworkManager::workerThread.isRunning());
+
+    for (const auto code : codes)
+    {
+        auto url = getStatusURL(code);
+        std::mutex mut;
+        bool requestDone = false;
+        std::condition_variable requestDoneCondition;
+
+        NetworkRequest(url)
+            .onSuccess([code, &mut, &requestDone, &requestDoneCondition,
+                        url](NetworkResult result) -> Outcome {
+                // The codes should throw an error
+                EXPECT_TRUE(false);
+
+                {
+                    std::unique_lock lck(mut);
+                    requestDone = true;
+                }
+                requestDoneCondition.notify_one();
+                return Success;
+            })
+            .onError([code, &mut, &requestDone, &requestDoneCondition,
+                      url](NetworkResult result) {
+                EXPECT_EQ(result.status(), code);
+
+                {
+                    std::unique_lock lck(mut);
+                    requestDone = true;
+                }
+                requestDoneCondition.notify_one();
+            })
+            .execute();
+
+        // Wait for the request to finish
+        std::unique_lock lck(mut);
+        requestDoneCondition.wait(lck, [&requestDone] {
+            return requestDone;
+        });
+    }
+
+    EXPECT_TRUE(NetworkManager::workerThread.isRunning());
+}
+
+TEST(NetworkRequest, TimeoutTimingOut)
+{
+    EXPECT_TRUE(NetworkManager::workerThread.isRunning());
+
+    auto url = "http://httpbin.org/delay/5";
+
+    std::mutex mut;
+    bool requestDone = false;
+    std::condition_variable requestDoneCondition;
+
+    NetworkRequest(url)
+        .timeout(1000)
+        .onSuccess([&mut, &requestDone, &requestDoneCondition,
+                    url](NetworkResult result) -> Outcome {
+            // The timeout should throw an error
+            EXPECT_TRUE(false);
+
+            {
+                std::unique_lock lck(mut);
+                requestDone = true;
+            }
+            requestDoneCondition.notify_one();
+            return Success;
+        })
+        .onError([&mut, &requestDone, &requestDoneCondition,
+                  url](NetworkResult result) {
+            qDebug() << QTime::currentTime().toString()
+                     << "timeout request finish error";
+            EXPECT_EQ(result.status(), NetworkResult::timedoutStatus);
+
+            {
+                std::unique_lock lck(mut);
+                requestDone = true;
+            }
+            requestDoneCondition.notify_one();
+        })
+        .execute();
+
+    // Wait for the request to finish
+    std::unique_lock lck(mut);
+    requestDoneCondition.wait(lck, [&requestDone] {
+        return requestDone;
+    });
+
+    EXPECT_TRUE(NetworkManager::workerThread.isRunning());
+}
+
+TEST(NetworkRequest, TimeoutNotTimingOut)
+{
+    EXPECT_TRUE(NetworkManager::workerThread.isRunning());
+
+    auto url = "http://httpbin.org/delay/1";
+
+    std::mutex mut;
+    bool requestDone = false;
+    std::condition_variable requestDoneCondition;
+
+    NetworkRequest(url)
+        .timeout(2000)
+        .onSuccess([&mut, &requestDone, &requestDoneCondition,
+                    url](NetworkResult result) -> Outcome {
+            EXPECT_EQ(result.status(), 200);
+
+            {
+                std::unique_lock lck(mut);
+                requestDone = true;
+            }
+            requestDoneCondition.notify_one();
+            return Success;
+        })
+        .onError([&mut, &requestDone, &requestDoneCondition,
+                  url](NetworkResult result) {
+            // The timeout should *not* throw an error
+            EXPECT_TRUE(false);
+
+            {
+                std::unique_lock lck(mut);
+                requestDone = true;
+            }
+            requestDoneCondition.notify_one();
+        })
+        .execute();
+
+    // Wait for the request to finish
+    std::unique_lock lck(mut);
+    requestDoneCondition.wait(lck, [&requestDone] {
+        return requestDone;
+    });
+
+    EXPECT_TRUE(NetworkManager::workerThread.isRunning());
+}

tests/src/main.cpp

@@ -1,3 +1,69 @@
+#include "common/NetworkManager.hpp"
+#include "common/NetworkRequest.hpp"
+#include "common/NetworkResult.hpp"
+#include "common/Outcome.hpp"
+#include "providers/twitch/api/Helix.hpp"
+
+#include "common/Outcome.hpp"
+#include "common/QLogging.hpp"
+
 #include <gtest/gtest.h>
 
-// do nothing
+#include <QJsonArray>
+#include <QtConcurrent>
+
+#include <chrono>
+#include <thread>
+#include "common/NetworkManager.hpp"
+
+#include <gtest/gtest.h>
+#include <QApplication>
+#include <QTimer>
+
+using namespace chatterino;
+
+void xd()
+{
+    std::mutex mut;
+    bool requestDone = false;
+    std::condition_variable requestDoneCondition;
+
+    EXPECT_TRUE(NetworkManager::workerThread.isRunning());
+
+    using namespace std::chrono_literals;
+
+    auto url = "http://localhost:8000/status/200";
+    NetworkRequest(url)
+        .onSuccess([&](NetworkResult result) -> Outcome {
+            qDebug() << "ON SUCCESS";
+            qDebug() << url;
+            return Success;
+        })
+        .onError([&](NetworkResult result) {
+            qDebug() << "ON ERROR";
+        })
+        .execute();
+
+    EXPECT_TRUE(NetworkManager::workerThread.isRunning());
+
+    EXPECT_TRUE(true);
+}
+
+int main(int argc, char **argv)
+{
+    ::testing::InitGoogleTest(&argc, argv);
+
+    QApplication app(argc, argv);
+
+    chatterino::NetworkManager::init();
+
+    QtConcurrent::run([&app] {
+        auto res = RUN_ALL_TESTS();
+
+        chatterino::NetworkManager::deinit();
+
+        app.exit(res);
+    });
+
+    return app.exec();
+}