From 8f170b8a69ced242cd5ff9bae8c3edfb39aa2357 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Thu, 24 Mar 2016 11:52:35 +0000 Subject: [PATCH] Improve GitHelper tests and refactor IsFileDifferent() --- CMakeLists.txt | 2 +- src/backend/helpers/git_helper.cpp | 7 +- src/backend/helpers/git_helper.h | 4 +- src/backend/masterlist.cpp | 4 +- .../{test_git_helper.h => git_helper_test.h} | 161 ++++++++++-------- src/tests/main.cpp | 2 +- 6 files changed, 95 insertions(+), 85 deletions(-) rename src/tests/backend/helpers/{test_git_helper.h => git_helper_test.h} (53%) diff --git a/CMakeLists.txt b/CMakeLists.txt index 767fd2cf..19af5db1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -239,7 +239,7 @@ set (LOOT_TESTS_HEADERS "${CMAKE_SOURCE_DIR}/src/tests/base_game_test.h" "${CMAKE_SOURCE_DIR}/src/tests/backend/game/game_cache_test.h" "${CMAKE_SOURCE_DIR}/src/tests/backend/game/game_settings_test.h" "${CMAKE_SOURCE_DIR}/src/tests/backend/game/load_order_handler_test.h" - "${CMAKE_SOURCE_DIR}/src/tests/backend/helpers/test_git_helper.h" + "${CMAKE_SOURCE_DIR}/src/tests/backend/helpers/git_helper_test.h" "${CMAKE_SOURCE_DIR}/src/tests/backend/helpers/test_helpers.h" "${CMAKE_SOURCE_DIR}/src/tests/backend/helpers/test_language.h" "${CMAKE_SOURCE_DIR}/src/tests/backend/helpers/test_version.h" diff --git a/src/backend/helpers/git_helper.cpp b/src/backend/helpers/git_helper.cpp index 88608072..53e9baa0 100644 --- a/src/backend/helpers/git_helper.cpp +++ b/src/backend/helpers/git_helper.cpp @@ -316,15 +316,14 @@ namespace loot { return revision; } - bool IsFileDifferent(const boost::filesystem::path& repoRoot, const std::string& filename) { - GitHelper git; - - if (!git.IsRepository(repoRoot)) { + bool GitHelper::IsFileDifferent(const boost::filesystem::path& repoRoot, const std::string& filename) { + if (!IsRepository(repoRoot)) { BOOST_LOG_TRIVIAL(info) << "Unknown masterlist revision: Git repository missing."; throw error(error::ok, lc::translate("Unknown: Git repository missing")); } BOOST_LOG_TRIVIAL(debug) << "Existing repository found, attempting to open it."; + GitHelper git; git.Call(git_repository_open(&git.repo, repoRoot.string().c_str())); // Perform a git diff, then iterate the deltas to see if one exists for the masterlist. diff --git a/src/backend/helpers/git_helper.h b/src/backend/helpers/git_helper.h index 4250f42e..0e115f77 100644 --- a/src/backend/helpers/git_helper.h +++ b/src/backend/helpers/git_helper.h @@ -43,6 +43,8 @@ namespace loot { static bool IsRepository(const boost::filesystem::path& path); + static bool IsFileDifferent(const boost::filesystem::path& repoRoot, const std::string& filename); + // Clones a repository and opens it. Sets 'repo'. void Clone(const boost::filesystem::path& path, const std::string& url); @@ -85,7 +87,5 @@ namespace loot { // created by libgit2. static void FixRepoPermissions(const boost::filesystem::path& path); }; - - bool IsFileDifferent(const boost::filesystem::path& repoRoot, const std::string& filename); } #endif diff --git a/src/backend/masterlist.cpp b/src/backend/masterlist.cpp index 621f1bc1..e1303b58 100644 --- a/src/backend/masterlist.cpp +++ b/src/backend/masterlist.cpp @@ -77,7 +77,7 @@ namespace loot { info.date = out.str(); BOOST_LOG_TRIVIAL(trace) << "Diffing masterlist HEAD and working copy."; - if (IsFileDifferent(path.parent_path(), path.filename().string())) { + if (GitHelper::IsFileDifferent(path.parent_path(), path.filename().string())) { info.revision += string(" ") + lc::translate("(edited)").str(); info.date += string(" ") + lc::translate("(edited)").str(); } @@ -203,7 +203,7 @@ namespace loot { // must be performed and the checked-out file parsed. if (!updateBranchHead) { BOOST_LOG_TRIVIAL(trace) << "Local and remote branch heads are equal."; - if (!IsFileDifferent(repoPath, filename)) { + if (!GitHelper::IsFileDifferent(repoPath, filename)) { BOOST_LOG_TRIVIAL(info) << "Local branch and masterlist file are already up to date."; return false; } diff --git a/src/tests/backend/helpers/test_git_helper.h b/src/tests/backend/helpers/git_helper_test.h similarity index 53% rename from src/tests/backend/helpers/test_git_helper.h rename to src/tests/backend/helpers/git_helper_test.h index ef40d144..23a7469f 100644 --- a/src/tests/backend/helpers/test_git_helper.h +++ b/src/tests/backend/helpers/git_helper_test.h @@ -22,81 +22,22 @@ along with LOOT. If not, see . */ -#ifndef LOOT_TEST_BACKEND_HELPERS_LANGUAGE -#define LOOT_TEST_BACKEND_HELPERS_LANGUAGE +#ifndef LOOT_TEST_BACKEND_HELPERS_GIT_HELPER +#define LOOT_TEST_BACKEND_HELPERS_GIT_HELPER #include "backend/error.h" #include "backend/helpers/git_helper.h" -#include "tests/fixtures.h" + +#include namespace loot { namespace test { - boost::filesystem::path getRepoRoot() { - boost::filesystem::path dir = boost::filesystem::current_path(); - while (!boost::filesystem::exists(dir / ".git")) { - dir = dir.parent_path(); - } + class GitHelperTest : public ::testing::Test { + protected: + GitHelperTest() : + parentRepoRoot(getRepoRoot()) {} - return dir; - } - - boost::filesystem::path parentRepoRoot = getRepoRoot(); - - TEST(GitHelper, ConstructorAndDestructor) { - GitHelper * git = new GitHelper(); - EXPECT_EQ(nullptr, git->repo); - - EXPECT_EQ(2, git_libgit2_init()); - delete git; - EXPECT_EQ(0, git_libgit2_shutdown()); - } - - TEST(GitHelper, Call) { - GitHelper git; - EXPECT_NO_THROW(git.Call(0)); - EXPECT_THROW(git.Call(1), error); - EXPECT_THROW(git.Call(-1), error); - } - - TEST(GitHelper, SetErrorMessage) { - GitHelper git; - git.SetErrorMessage("test message"); - - try { - git.Call(1); - ADD_FAILURE() << "An exception should have been thrown."; - } - catch (error& e) { - EXPECT_NE(nullptr, strstr(e.what(), "test message")); - } - } - - TEST(GitHelper, Free) { - // Initialise buffer member, it's simplest to test with. - GitHelper git; - git_buf_set(&git.buf, "foo", 4); - EXPECT_NE(nullptr, git.buf.ptr); - EXPECT_EQ(4, git.buf.size); - - git.Free(); - EXPECT_EQ(nullptr, git.buf.ptr); - EXPECT_EQ(0, git.buf.size); - } - - TEST(GitHelper, IsRepository) { - GitHelper git; - - ASSERT_TRUE(boost::filesystem::exists(parentRepoRoot)); - EXPECT_TRUE(git.IsRepository(parentRepoRoot)); - EXPECT_FALSE(git.IsRepository(boost::filesystem::current_path())); - } - - class IsFileDifferentTest : public ::testing::Test { inline virtual void SetUp() { - GitHelper git; - ASSERT_TRUE(git.IsRepository(parentRepoRoot)); - ASSERT_FALSE(git.IsRepository(boost::filesystem::current_path())); - ASSERT_TRUE(boost::filesystem::exists(parentRepoRoot / "README.md")); // Create a backup of CONTRIBUTING.md. @@ -117,24 +58,94 @@ namespace loot { ASSERT_TRUE(boost::filesystem::exists(parentRepoRoot / "CONTRIBUTING.md")); ASSERT_FALSE(boost::filesystem::exists(parentRepoRoot / "CONTRIBUTING.md.copy")); } + + GitHelper git; + + const boost::filesystem::path parentRepoRoot; + + private: + inline static boost::filesystem::path getRepoRoot() { + boost::filesystem::path dir = boost::filesystem::current_path(); + while (!boost::filesystem::exists(dir / ".git")) { + dir = dir.parent_path(); + } + + return dir; + } }; - TEST_F(IsFileDifferentTest, InvalidRepository) { - EXPECT_THROW(IsFileDifferent(boost::filesystem::current_path(), "README.md"), error); + TEST_F(GitHelperTest, repoShouldInitialiseAsANullPointer) { + EXPECT_EQ(nullptr, git.repo); } - TEST_F(IsFileDifferentTest, SameFile) { - EXPECT_FALSE(IsFileDifferent(parentRepoRoot, "README.md")); + TEST_F(GitHelperTest, destructorShouldCallLibgit2CleanupFunction) { + ASSERT_EQ(2, git_libgit2_init()); + + GitHelper * gitPointer = new GitHelper(); + ASSERT_EQ(4, git_libgit2_init()); + + delete gitPointer; + EXPECT_EQ(2, git_libgit2_shutdown()); } - TEST_F(IsFileDifferentTest, NewFile) { + TEST_F(GitHelperTest, callShouldNotThrowIfPassedAZeroValue) { + EXPECT_NO_THROW(git.Call(0)); + } + + TEST_F(GitHelperTest, callShouldThrowIfPassedANonZeroValue) { + EXPECT_THROW(git.Call(1), error); + EXPECT_THROW(git.Call(-1), error); + } + + TEST_F(GitHelperTest, setErrorMessageShouldSetTheMessageForThrownExceptions) { + const char * errorMessage = "test message"; + git.SetErrorMessage(errorMessage); + + try { + git.Call(1); + ADD_FAILURE() << "An exception should have been thrown."; + } + catch (error& e) { + EXPECT_NE(nullptr, strstr(e.what(), errorMessage)); + } + } + + TEST_F(GitHelperTest, freeShouldFreeMemoryAllocatedToMembers) { + // Initialise buffer member, it's simplest to test with. + GitHelper git; + git_buf_set(&git.buf, "foo", 4); + ASSERT_NE(nullptr, git.buf.ptr); + ASSERT_EQ(4, git.buf.size); + + git.Free(); + EXPECT_EQ(nullptr, git.buf.ptr); + EXPECT_EQ(0, git.buf.size); + } + + TEST_F(GitHelperTest, isRepositoryShouldReturnTrueForARepositoryRoot) { + EXPECT_TRUE(GitHelper::IsRepository(parentRepoRoot)); + } + + TEST_F(GitHelperTest, isRepositoryShouldReturnFalseForRepositorySubdirectory) { + EXPECT_FALSE(GitHelper::IsRepository(boost::filesystem::current_path())); + } + + TEST_F(GitHelperTest, isFileDifferentShouldThrowIfGivenANonRepositoryPath) { + EXPECT_THROW(GitHelper::IsFileDifferent(boost::filesystem::current_path(), "README.md"), error); + } + + TEST_F(GitHelperTest, isFileDifferentShouldReturnFalseForAnUntrackedFile) { // New files not in the index are not tracked by Git, so aren't considered // different. - EXPECT_FALSE(IsFileDifferent(parentRepoRoot, "CONTRIBUTING.md.copy")); + EXPECT_FALSE(GitHelper::IsFileDifferent(parentRepoRoot, "CONTRIBUTING.md.copy")); } - TEST_F(IsFileDifferentTest, DifferentFile) { - EXPECT_TRUE(IsFileDifferent(parentRepoRoot, "CONTRIBUTING.md")); + TEST_F(GitHelperTest, isFileDifferentShouldReturnFalseForAnUnchangedTrackedFile) { + EXPECT_FALSE(GitHelper::IsFileDifferent(parentRepoRoot, "README.md")); + } + + TEST_F(GitHelperTest, isFileDifferentShouldReturnTrueForAChangedTrackedFile) { + EXPECT_TRUE(GitHelper::IsFileDifferent(parentRepoRoot, "CONTRIBUTING.md")); } } } diff --git a/src/tests/main.cpp b/src/tests/main.cpp index 63a8c294..bf8c7199 100644 --- a/src/tests/main.cpp +++ b/src/tests/main.cpp @@ -45,7 +45,7 @@ #include "backend/game/game_cache_test.h" #include "backend/game/game_settings_test.h" #include "backend/game/load_order_handler_test.h" -#include "backend/helpers/test_git_helper.h" +#include "backend/helpers/git_helper_test.h" #include "backend/helpers/test_helpers.h" #include "backend/helpers/test_language.h" #include "backend/helpers/test_version.h"