From 0e66b17ff071f3ec3c6aeb5983be8a61f8705fdf Mon Sep 17 00:00:00 2001 From: pajlada Date: Sat, 26 Dec 2020 12:42:39 +0100 Subject: [PATCH] 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 | 30 ++- lib/settings | 2 +- src/common/Common.hpp | 8 + src/common/NetworkPrivate.cpp | 41 ++-- src/common/NetworkPrivate.hpp | 5 +- src/common/NetworkRequest.cpp | 2 +- src/common/NetworkResult.hpp | 1 + src/controllers/filters/FilterRecord.hpp | 1 + src/controllers/filters/parser/Tokenizer.hpp | 4 + src/providers/twitch/TwitchCommon.hpp | 8 + src/providers/twitch/api/Helix.hpp | 1 + src/util/QObjectRef.hpp | 1 + tests/.clang-format | 1 + tests/src/NetworkRequest.cpp | 211 +++++++++++++++++++ tests/src/main.cpp | 68 +++++- 15 files changed, 362 insertions(+), 22 deletions(-) create mode 100644 tests/src/NetworkRequest.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index de20932b0..d6b3b7fe1 100644 --- a/CMakeLists.txt +++ b/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() diff --git a/lib/settings b/lib/settings index 6de3a2778..cc2fb43ad 160000 --- a/lib/settings +++ b/lib/settings @@ -1 +1 @@ -Subproject commit 6de3a27784745959ab261df219880cddc9263ff9 +Subproject commit cc2fb43ad515e8ff7349e2eb4ddeb3fb96337b1a diff --git a/src/common/Common.hpp b/src/common/Common.hpp index 43b954078..eef9d0467 100644 --- a/src/common/Common.hpp +++ b/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 diff --git a/src/common/NetworkPrivate.cpp b/src/common/NetworkPrivate.cpp index dc1ffd436..347280e15 100644 --- a/src/common/NetworkPrivate.cpp +++ b/src/common/NetworkPrivate.cpp @@ -10,23 +10,20 @@ #include #include +#include #include #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 &data) worker->moveToThread(&NetworkManager::workerThread); - if (data->hasTimeout_) - { - data->timer_->setSingleShot(true); - data->timer_->start(); - } - auto onUrlRequested = [data, worker]() mutable { + if (data->hasTimeout_) + { + data->timer_ = new QTimer(); + data->timer_->setSingleShot(true); + data->timer_->start(data->timeoutMS_); + } + auto reply = [&]() -> QNetworkReply * { switch (data->requestType_) { @@ -128,7 +126,7 @@ void loadUncached(const std::shared_ptr &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 &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(); - postToThread([data, error] { - data->onError_(NetworkResult({}, error)); + auto status = reply->attribute( + QNetworkRequest::HttpStatusCodeAttribute); + postToThread([data, code = status.toInt()] { + data->onError_(NetworkResult({}, code)); }); } return; @@ -193,6 +198,12 @@ void loadUncached(const std::shared_ptr &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 { diff --git a/src/common/NetworkPrivate.hpp b/src/common/NetworkPrivate.hpp index 1009c556f..c25439f0c 100644 --- a/src/common/NetworkPrivate.hpp +++ b/src/common/NetworkPrivate.hpp @@ -5,7 +5,9 @@ #include #include +#include #include +#include 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(); diff --git a/src/common/NetworkRequest.cpp b/src/common/NetworkRequest.cpp index 1041fafe5..c39100b1b 100644 --- a/src/common/NetworkRequest.cpp +++ b/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); } diff --git a/src/common/NetworkResult.hpp b/src/common/NetworkResult.hpp index a0dae5455..4edd6ad4a 100644 --- a/src/common/NetworkResult.hpp +++ b/src/common/NetworkResult.hpp @@ -1,6 +1,7 @@ #pragma once #include +#include #include namespace chatterino { diff --git a/src/controllers/filters/FilterRecord.hpp b/src/controllers/filters/FilterRecord.hpp index f0942df31..76288e868 100644 --- a/src/controllers/filters/FilterRecord.hpp +++ b/src/controllers/filters/FilterRecord.hpp @@ -8,6 +8,7 @@ #include #include +#include #include #include diff --git a/src/controllers/filters/parser/Tokenizer.hpp b/src/controllers/filters/parser/Tokenizer.hpp index dcab024de..38a0a1cb7 100644 --- a/src/controllers/filters/parser/Tokenizer.hpp +++ b/src/controllers/filters/parser/Tokenizer.hpp @@ -2,6 +2,10 @@ #include "controllers/filters/parser/Types.hpp" +#include +#include +#include + namespace filterparser { static const QMap validIdentifiersMap = { diff --git a/src/providers/twitch/TwitchCommon.hpp b/src/providers/twitch/TwitchCommon.hpp index 329da1074..6c41dbdc4 100644 --- a/src/providers/twitch/TwitchCommon.hpp +++ b/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() diff --git a/src/providers/twitch/api/Helix.hpp b/src/providers/twitch/api/Helix.hpp index bea88b258..e9730a43f 100644 --- a/src/providers/twitch/api/Helix.hpp +++ b/src/providers/twitch/api/Helix.hpp @@ -2,6 +2,7 @@ #include "common/NetworkRequest.hpp" +#include #include #include #include diff --git a/src/util/QObjectRef.hpp b/src/util/QObjectRef.hpp index f3a985eea..f434f4a3a 100644 --- a/src/util/QObjectRef.hpp +++ b/src/util/QObjectRef.hpp @@ -1,5 +1,6 @@ #pragma once +#include #include #include diff --git a/tests/.clang-format b/tests/.clang-format index 0ef59b1fe..f34c1465b 100644 --- a/tests/.clang-format +++ b/tests/.clang-format @@ -4,6 +4,7 @@ AccessModifierOffset: -4 AlignEscapedNewlinesLeft: true AllowShortFunctionsOnASingleLine: false AllowShortIfStatementsOnASingleLine: false +AllowShortLambdasOnASingleLine: Empty AllowShortLoopsOnASingleLine: false AlwaysBreakAfterDefinitionReturnType: false AlwaysBreakBeforeMultilineStrings: false diff --git a/tests/src/NetworkRequest.cpp b/tests/src/NetworkRequest.cpp new file mode 100644 index 000000000..8a04a11b3 --- /dev/null +++ b/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 + +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 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 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()); +} diff --git a/tests/src/main.cpp b/tests/src/main.cpp index db640b12d..9a55758bf 100644 --- a/tests/src/main.cpp +++ b/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 -// do nothing +#include +#include + +#include +#include +#include "common/NetworkManager.hpp" + +#include +#include +#include + +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(); +}