From bf0d3aa37e4b5802e26937a628347a79d49e94ba Mon Sep 17 00:00:00 2001 From: Jojo-1000 <33495614+Jojo-1000@users.noreply.github.com> Date: Fri, 27 Apr 2018 23:03:39 +0200 Subject: [PATCH] Implement retry in HueCommandAPI, add tests. - On errors connection reset or timed out, waits and retries once --- hueplusplus/Hue.cpp | 21 ++++++++------------- hueplusplus/HueCommandAPI.cpp | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------- hueplusplus/test/CMakeLists.txt | 1 + hueplusplus/test/test_Hue.cpp | 1 + hueplusplus/test/test_HueCommandAPI.cpp | 200 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 274 insertions(+), 26 deletions(-) create mode 100644 hueplusplus/test/test_HueCommandAPI.cpp diff --git a/hueplusplus/Hue.cpp b/hueplusplus/Hue.cpp index f44ea3c..df9867b 100755 --- a/hueplusplus/Hue.cpp +++ b/hueplusplus/Hue.cpp @@ -79,21 +79,16 @@ Hue HueFinder::GetBridge(const HueIdentification& identification) { return Hue(identification.ip, pos->second, http_handler); } - else + Hue bridge(identification.ip, "", http_handler); + bridge.requestUsername(identification.ip); + if (bridge.getUsername().empty()) { - Hue bridge(identification.ip, "", http_handler); - bridge.requestUsername(identification.ip); - if (bridge.getUsername().empty()) - { - std::cerr << "Failed to request username for ip " << identification.ip << std::endl; - throw std::runtime_error("Failed to request username!"); - } - else - { - AddUsername(identification.mac, bridge.getUsername()); - } - return bridge; + std::cerr << "Failed to request username for ip " << identification.ip << std::endl; + throw std::runtime_error("Failed to request username!"); } + AddUsername(identification.mac, bridge.getUsername()); + + return bridge; } void HueFinder::AddUsername(const std::string& mac, const std::string& username) diff --git a/hueplusplus/HueCommandAPI.cpp b/hueplusplus/HueCommandAPI.cpp index deb5d50..bb5a09f 100644 --- a/hueplusplus/HueCommandAPI.cpp +++ b/hueplusplus/HueCommandAPI.cpp @@ -27,7 +27,7 @@ HueCommandAPI::HueCommandAPI(const std::string& ip, const std::string& username, : ip(ip), username(username), httpHandler(std::move(httpHandler)), - timeout(new TimeoutData{std::chrono::steady_clock::now()}) + timeout(new TimeoutData{ std::chrono::steady_clock::now() }) {} Json::Value HueCommandAPI::PUTRequest(const std::string& path, const Json::Value& request) const @@ -38,10 +38,27 @@ Json::Value HueCommandAPI::PUTRequest(const std::string& path, const Json::Value { std::this_thread::sleep_until(timeout->timeout); } - Json::Value v = httpHandler->PUTJson("/api/" + username + path, request, ip); - timeout->timeout = now + minDelay; - - return v; + //If path does not begin with '/', insert it unless path is empty + const std::string combinedPath = "/api/" + username + (path.empty() || path.front() == '/' ? "" : "/") + path; + try + { + Json::Value v = httpHandler->PUTJson(combinedPath, request, ip); + timeout->timeout = now + minDelay; + return v; + } + catch (const std::system_error& e) + { + if (e.code() == std::errc::connection_reset || e.code() == std::errc::timed_out) + { + //Happens when hue is too busy, wait and try again (once) + std::this_thread::sleep_for(minDelay); + Json::Value v = httpHandler->PUTJson(combinedPath, request, ip); + timeout->timeout = std::chrono::steady_clock::now() + minDelay; + return v; + } + //Cannot recover from other types of errors + throw; + } } Json::Value HueCommandAPI::GETRequest(const std::string& path, const Json::Value& request) const @@ -52,10 +69,27 @@ Json::Value HueCommandAPI::GETRequest(const std::string& path, const Json::Value { std::this_thread::sleep_until(timeout->timeout); } - Json::Value v = httpHandler->GETJson("/api/" + username + path, request, ip); - timeout->timeout = now + minDelay; - - return v; + //If path does not begin with '/', insert it unless path is empty + const std::string combinedPath = "/api/" + username + (path.empty() || path.front() == '/' ? "" : "/") + path; + try + { + Json::Value v = httpHandler->GETJson(combinedPath, request, ip); + timeout->timeout = now + minDelay; + return v; + } + catch (const std::system_error& e) + { + if (e.code() == std::errc::connection_reset || e.code() == std::errc::timed_out) + { + //Happens when hue is too busy, wait and try again (once) + std::this_thread::sleep_for(minDelay); + Json::Value v = httpHandler->GETJson(combinedPath, request, ip); + timeout->timeout = std::chrono::steady_clock::now() + minDelay; + return v; + } + //Cannot recover from other types of errors + throw; + } } Json::Value HueCommandAPI::DELETERequest(const std::string& path, const Json::Value& request) const @@ -66,8 +100,25 @@ Json::Value HueCommandAPI::DELETERequest(const std::string& path, const Json::Va { std::this_thread::sleep_until(timeout->timeout); } - Json::Value v = httpHandler->DELETEJson("/api/" + username + path, request, ip); - timeout->timeout = now + minDelay; - - return v; + //If path does not begin with '/', insert it unless path is empty + const std::string combinedPath = "/api/" + username + (path.empty() || path.front() == '/' ? "" : "/") + path; + try + { + Json::Value v = httpHandler->DELETEJson(combinedPath, request, ip); + timeout->timeout = now + minDelay; + return v; + } + catch (const std::system_error& e) + { + if (e.code() == std::errc::connection_reset || e.code() == std::errc::timed_out) + { + //Happens when hue is too busy, wait and try again (once) + std::this_thread::sleep_for(minDelay); + Json::Value v = httpHandler->DELETEJson(combinedPath, request, ip); + timeout->timeout = std::chrono::steady_clock::now() + minDelay; + return v; + } + //Cannot recover from other types of errors + throw; + } } diff --git a/hueplusplus/test/CMakeLists.txt b/hueplusplus/test/CMakeLists.txt index 8f4b41f..c5dc208 100755 --- a/hueplusplus/test/CMakeLists.txt +++ b/hueplusplus/test/CMakeLists.txt @@ -39,6 +39,7 @@ set(TEST_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/test_ExtendedColorTemperatureStrategy.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_Hue.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_HueLight.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/test_HueCommandAPI.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_Main.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_SimpleBrightnessStrategy.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_SimpleColorHueStrategy.cpp diff --git a/hueplusplus/test/test_Hue.cpp b/hueplusplus/test/test_Hue.cpp index 6c92e51..9a692ec 100755 --- a/hueplusplus/test/test_Hue.cpp +++ b/hueplusplus/test/test_Hue.cpp @@ -400,6 +400,7 @@ TEST(Hue, getAllLights) Hue test_bridge(bridge_ip, bridge_username, handler); std::vector> test_lights = test_bridge.getAllLights(); + ASSERT_EQ(1, test_lights.size()); EXPECT_EQ(test_lights[0].get().getName(), "Hue ambiance lamp 1"); EXPECT_EQ(test_lights[0].get().getColorType(), ColorType::TEMPERATURE); } diff --git a/hueplusplus/test/test_HueCommandAPI.cpp b/hueplusplus/test/test_HueCommandAPI.cpp new file mode 100644 index 0000000..894afa1 --- /dev/null +++ b/hueplusplus/test/test_HueCommandAPI.cpp @@ -0,0 +1,200 @@ +/** + \file test_HueCommandAPI.cpp + Copyright Notice\n + Copyright (C) 2018 Jan Rogall - developer\n + Copyright (C) 2018 Moritz Wirger - developer\n + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + This program 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, write to the Free Software Foundation, + Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +**/ + +#include +#include + +#include "../include/Hue.h" +#include "../include/json/json.h" +#include "mocks/mock_HttpHandler.h" +#include "testhelper.h" + +TEST(HueCommandAPI, PUTRequest) +{ + using namespace ::testing; + std::shared_ptr httpHandler = std::make_shared(); + + HueCommandAPI api(bridge_ip, bridge_username, httpHandler); + Json::Value request; + Json::Value result = Json::objectValue; + result["ok"] = true; + + //empty path + { + EXPECT_CALL(*httpHandler, PUTJson("/api/" + bridge_username, request, bridge_ip, 80)).WillOnce(Return(result)); + EXPECT_EQ(result, api.PUTRequest("", request)); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //not empty path, starting with slash + { + const std::string path = "/test"; + EXPECT_CALL(*httpHandler, PUTJson("/api/" + bridge_username + path, request, bridge_ip, 80)).WillOnce(Return(result)); + EXPECT_EQ(result, api.PUTRequest(path, request)); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //not empty path, not starting with slash + { + const std::string path = "test"; + EXPECT_CALL(*httpHandler, PUTJson("/api/" + bridge_username + '/' + path, request, bridge_ip, 80)).WillOnce(Return(result)); + EXPECT_EQ(result, api.PUTRequest(path, request)); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //recoverable error + { + const std::string path = "/test"; + EXPECT_CALL(*httpHandler, PUTJson("/api/" + bridge_username + path, request, bridge_ip, 80)) + .WillOnce(Throw(std::system_error(std::make_error_code(std::errc::connection_reset)))) + .WillOnce(Return(result)); + EXPECT_EQ(result, api.PUTRequest(path, request)); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //recoverable error x2 + { + const std::string path = "/test"; + EXPECT_CALL(*httpHandler, PUTJson("/api/" + bridge_username + path, request, bridge_ip, 80)) + .WillOnce(Throw(std::system_error(std::make_error_code(std::errc::connection_reset)))) + .WillOnce(Throw(std::system_error(std::make_error_code(std::errc::connection_reset)))); + EXPECT_THROW(api.PUTRequest(path, request), std::system_error); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //unrecoverable error + { + const std::string path = "/test"; + EXPECT_CALL(*httpHandler, PUTJson("/api/" + bridge_username + path, request, bridge_ip, 80)) + .WillOnce(Throw(std::system_error(std::make_error_code(std::errc::not_enough_memory)))); + EXPECT_THROW(api.PUTRequest(path, request), std::system_error); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } +} + +TEST(HueCommandAPI, GETRequest) +{ + using namespace ::testing; + std::shared_ptr httpHandler = std::make_shared(); + + HueCommandAPI api(bridge_ip, bridge_username, httpHandler); + Json::Value request; + Json::Value result = Json::objectValue; + result["ok"] = true; + + //empty path + { + EXPECT_CALL(*httpHandler, GETJson("/api/" + bridge_username, request, bridge_ip, 80)).WillOnce(Return(result)); + EXPECT_EQ(result, api.GETRequest("", request)); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //not empty path, starting with slash + { + const std::string path = "/test"; + EXPECT_CALL(*httpHandler, GETJson("/api/" + bridge_username + path, request, bridge_ip, 80)).WillOnce(Return(result)); + EXPECT_EQ(result, api.GETRequest(path, request)); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //not empty path, not starting with slash + { + const std::string path = "test"; + EXPECT_CALL(*httpHandler, GETJson("/api/" + bridge_username + '/' + path, request, bridge_ip, 80)).WillOnce(Return(result)); + EXPECT_EQ(result, api.GETRequest(path, request)); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //recoverable error + { + const std::string path = "/test"; + EXPECT_CALL(*httpHandler, GETJson("/api/" + bridge_username + path, request, bridge_ip, 80)) + .WillOnce(Throw(std::system_error(std::make_error_code(std::errc::connection_reset)))) + .WillOnce(Return(result)); + EXPECT_EQ(result, api.GETRequest(path, request)); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //recoverable error x2 + { + const std::string path = "/test"; + EXPECT_CALL(*httpHandler, GETJson("/api/" + bridge_username + path, request, bridge_ip, 80)) + .WillOnce(Throw(std::system_error(std::make_error_code(std::errc::connection_reset)))) + .WillOnce(Throw(std::system_error(std::make_error_code(std::errc::connection_reset)))); + EXPECT_THROW(api.GETRequest(path, request), std::system_error); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //unrecoverable error + { + const std::string path = "/test"; + EXPECT_CALL(*httpHandler, GETJson("/api/" + bridge_username + path, request, bridge_ip, 80)) + .WillOnce(Throw(std::system_error(std::make_error_code(std::errc::not_enough_memory)))); + EXPECT_THROW(api.GETRequest(path, request), std::system_error); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } +} + +TEST(HueCommandAPI, DELETERequest) +{ + using namespace ::testing; + std::shared_ptr httpHandler = std::make_shared(); + + HueCommandAPI api(bridge_ip, bridge_username, httpHandler); + Json::Value request; + Json::Value result = Json::objectValue; + result["ok"] = true; + + //empty path + { + EXPECT_CALL(*httpHandler, DELETEJson("/api/" + bridge_username, request, bridge_ip, 80)).WillOnce(Return(result)); + EXPECT_EQ(result, api.DELETERequest("", request)); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //not empty path, starting with slash + { + const std::string path = "/test"; + EXPECT_CALL(*httpHandler, DELETEJson("/api/" + bridge_username + path, request, bridge_ip, 80)).WillOnce(Return(result)); + EXPECT_EQ(result, api.DELETERequest(path, request)); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //not empty path, not starting with slash + { + const std::string path = "test"; + EXPECT_CALL(*httpHandler, DELETEJson("/api/" + bridge_username + '/' + path, request, bridge_ip, 80)).WillOnce(Return(result)); + EXPECT_EQ(result, api.DELETERequest(path, request)); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //recoverable error + { + const std::string path = "/test"; + EXPECT_CALL(*httpHandler, DELETEJson("/api/" + bridge_username + path, request, bridge_ip, 80)) + .WillOnce(Throw(std::system_error(std::make_error_code(std::errc::connection_reset)))) + .WillOnce(Return(result)); + EXPECT_EQ(result, api.DELETERequest(path, request)); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //recoverable error x2 + { + const std::string path = "/test"; + EXPECT_CALL(*httpHandler, DELETEJson("/api/" + bridge_username + path, request, bridge_ip, 80)) + .WillOnce(Throw(std::system_error(std::make_error_code(std::errc::connection_reset)))) + .WillOnce(Throw(std::system_error(std::make_error_code(std::errc::connection_reset)))); + EXPECT_THROW(api.DELETERequest(path, request), std::system_error); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } + //unrecoverable error + { + const std::string path = "/test"; + EXPECT_CALL(*httpHandler, GETJson("/api/" + bridge_username + path, request, bridge_ip, 80)) + .WillOnce(Throw(std::system_error(std::make_error_code(std::errc::not_enough_memory)))); + EXPECT_THROW(api.GETRequest(path, request), std::system_error); + Mock::VerifyAndClearExpectations(httpHandler.get()); + } +} \ No newline at end of file -- libgit2 0.21.4