From b9934a45326378ef53bf21c6ea0c3e9c32b34415 Mon Sep 17 00:00:00 2001 From: nerix Date: Sat, 24 Jun 2023 15:03:27 +0200 Subject: [PATCH] Refactor tests and benchmarks (#4700) --- .github/workflows/test.yml | 5 +- CHANGELOG.md | 1 + CMakeLists.txt | 1 + benchmarks/src/main.cpp | 6 +- src/messages/Image.cpp | 4 +- tests/CMakeLists.txt | 8 +- tests/src/NetworkRequest.cpp | 257 +++++++++++++---------------------- tests/src/main.cpp | 6 +- 8 files changed, 109 insertions(+), 179 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7ef41f8b6..610cdf29c 100644 --- a/.github/workflows/test.yml +++ b/.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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 955c3776e..7cf8e80af 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/CMakeLists.txt b/CMakeLists.txt index ac4338044..af05f742b 100644 --- a/CMakeLists.txt +++ b/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) diff --git a/benchmarks/src/main.cpp b/benchmarks/src/main.cpp index 78ecf5b4d..5f8f67870 100644 --- a/benchmarks/src/main.cpp +++ b/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(); } diff --git a/src/messages/Image.cpp b/src/messages/Image.cpp index 71daf3425..34bd7489a 100644 --- a/src/messages/Image.cpp +++ b/src/messages/Image.cpp @@ -595,8 +595,8 @@ ImageExpirationPool::ImageExpirationPool() ImageExpirationPool &ImageExpirationPool::instance() { - static ImageExpirationPool instance; - return instance; + static auto *instance = new ImageExpirationPool; + return *instance; } void ImageExpirationPool::addImagePtr(ImagePtr imgPtr) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index e6f7b5bf4..09a9828bc 100644 --- a/tests/CMakeLists.txt +++ b/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 -# 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} -# ) +gtest_discover_tests(${PROJECT_NAME}) diff --git a/tests/src/NetworkRequest.cpp b/tests/src/NetworkRequest.cpp index 85ed0246b..2bcb5da9a 100644 --- a/tests/src/NetworkRequest.cpp +++ b/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 +#include 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; - bool requestDone = false; - std::condition_variable requestDoneCondition; + RequestWaiter waiter; 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) { + .onSuccess( + [code, &waiter, url](const NetworkResult &result) -> Outcome { + EXPECT_EQ(result.status(), code); + waiter.requestDone(); + return Success; + }) + .onError([&](const NetworkResult & /*result*/) { // The codes should *not* throw an error EXPECT_TRUE(false); - - { - std::unique_lock lck(mut); - requestDone = true; - } - requestDoneCondition.notify_one(); + waiter.requestDone(); }) .execute(); - // Wait for the request to finish - std::unique_lock lck(mut); - requestDoneCondition.wait(lck, [&requestDone] { - return requestDone; - }); + waiter.waitForRequest(); } EXPECT_TRUE(NetworkManager::workerThread.isRunning()); @@ -86,30 +111,18 @@ TEST(NetworkRequest, FinallyCallbackOnSuccess) for (const auto code : codes) { auto url = getStatusURL(code); - std::mutex mut; - bool requestDone = false; - std::condition_variable requestDoneCondition; + RequestWaiter waiter; bool finallyCalled = false; NetworkRequest(url) - .finally( - [&mut, &requestDone, &requestDoneCondition, &finallyCalled] { - finallyCalled = true; - - { - std::unique_lock lck(mut); - requestDone = true; - } - requestDoneCondition.notify_one(); - }) + .finally([&waiter, &finallyCalled] { + finallyCalled = true; + waiter.requestDone(); + }) .execute(); - // Wait for the request to finish - std::unique_lock lck(mut); - requestDoneCondition.wait(lck, [&requestDone] { - return requestDone; - }); + waiter.waitForRequest(); EXPECT_TRUE(finallyCalled); } @@ -127,45 +140,24 @@ TEST(NetworkRequest, Error) for (const auto code : codes) { auto url = getStatusURL(code); - std::mutex mut; - bool requestDone = false; - std::condition_variable requestDoneCondition; + RequestWaiter waiter; 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) { + .onSuccess( + [&waiter, url](const NetworkResult & /*result*/) -> Outcome { + // The codes should throw an error + EXPECT_TRUE(false); + waiter.requestDone(); + return Success; + }) + .onError([code, &waiter, url](const NetworkResult &result) { EXPECT_EQ(result.status(), code); - if (code == 402) - { - EXPECT_EQ(result.getData(), - QString("Fuck you, pay me!").toUtf8()); - } - { - std::unique_lock lck(mut); - requestDone = true; - } - requestDoneCondition.notify_one(); + waiter.requestDone(); }) .execute(); - // Wait for the request to finish - std::unique_lock lck(mut); - requestDoneCondition.wait(lck, [&requestDone] { - return requestDone; - }); + waiter.waitForRequest(); } EXPECT_TRUE(NetworkManager::workerThread.isRunning()); @@ -183,30 +175,18 @@ TEST(NetworkRequest, FinallyCallbackOnError) for (const auto code : codes) { auto url = getStatusURL(code); - std::mutex mut; - bool requestDone = false; - std::condition_variable requestDoneCondition; + RequestWaiter waiter; bool finallyCalled = false; NetworkRequest(url) - .finally( - [&mut, &requestDone, &requestDoneCondition, &finallyCalled] { - finallyCalled = true; - - { - std::unique_lock lck(mut); - requestDone = true; - } - requestDoneCondition.notify_one(); - }) + .finally([&waiter, &finallyCalled] { + finallyCalled = true; + waiter.requestDone(); + }) .execute(); - // Wait for the request to finish - std::unique_lock lck(mut); - requestDoneCondition.wait(lck, [&requestDone] { - return requestDone; - }); + waiter.waitForRequest(); EXPECT_TRUE(finallyCalled); } @@ -217,44 +197,26 @@ TEST(NetworkRequest, TimeoutTimingOut) EXPECT_TRUE(NetworkManager::workerThread.isRunning()); auto url = getDelayURL(5); - - std::mutex mut; - bool requestDone = false; - std::condition_variable requestDoneCondition; + RequestWaiter waiter; NetworkRequest(url) .timeout(1000) - .onSuccess([&mut, &requestDone, &requestDoneCondition, - url](NetworkResult result) -> Outcome { + .onSuccess([&waiter](const NetworkResult & /*result*/) -> Outcome { // The timeout should throw an error EXPECT_TRUE(false); - - { - std::unique_lock lck(mut); - requestDone = true; - } - requestDoneCondition.notify_one(); + waiter.requestDone(); return Success; }) - .onError([&mut, &requestDone, &requestDoneCondition, - url](NetworkResult result) { + .onError([&waiter, url](const 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(); + waiter.requestDone(); }) .execute(); - // Wait for the request to finish - std::unique_lock lck(mut); - requestDoneCondition.wait(lck, [&requestDone] { - return requestDone; - }); + waiter.waitForRequest(); EXPECT_TRUE(NetworkManager::workerThread.isRunning()); } @@ -264,42 +226,24 @@ TEST(NetworkRequest, TimeoutNotTimingOut) EXPECT_TRUE(NetworkManager::workerThread.isRunning()); auto url = getDelayURL(1); - - std::mutex mut; - bool requestDone = false; - std::condition_variable requestDoneCondition; + RequestWaiter waiter; NetworkRequest(url) - .timeout(2000) - .onSuccess([&mut, &requestDone, &requestDoneCondition, - url](NetworkResult result) -> Outcome { + .timeout(3000) + .onSuccess([&waiter, url](const NetworkResult &result) -> Outcome { EXPECT_EQ(result.status(), 200); - { - std::unique_lock lck(mut); - requestDone = true; - } - requestDoneCondition.notify_one(); + waiter.requestDone(); return Success; }) - .onError([&mut, &requestDone, &requestDoneCondition, - url](NetworkResult result) { + .onError([&waiter, url](const NetworkResult & /*result*/) { // The timeout should *not* throw an error EXPECT_TRUE(false); - - { - std::unique_lock lck(mut); - requestDone = true; - } - requestDoneCondition.notify_one(); + waiter.requestDone(); }) .execute(); - // Wait for the request to finish - std::unique_lock lck(mut); - requestDoneCondition.wait(lck, [&requestDone] { - return requestDone; - }); + waiter.waitForRequest(); EXPECT_TRUE(NetworkManager::workerThread.isRunning()); } @@ -310,39 +254,28 @@ TEST(NetworkRequest, FinallyCallbackOnTimeout) auto url = getDelayURL(5); - std::mutex mut; - bool requestDone = false; - std::condition_variable requestDoneCondition; + RequestWaiter waiter; 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; - - { - std::unique_lock lck(mut); - requestDone = true; - } - requestDoneCondition.notify_one(); + waiter.requestDone(); }) .execute(); - // Wait for the request to finish - std::unique_lock lck(mut); - requestDoneCondition.wait(lck, [&requestDone] { - return requestDone; - }); + waiter.waitForRequest(); EXPECT_TRUE(finallyCalled); EXPECT_TRUE(onErrorCalled); diff --git a/tests/src/main.cpp b/tests/src/main.cpp index 421ea6e33..c54ea158f 100644 --- a/tests/src/main.cpp +++ b/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