From e880f6ba1d6d5f19355f870bed51070280bbeb88 Mon Sep 17 00:00:00 2001 From: waffles Date: Wed, 25 Jun 2025 11:59:07 -0700 Subject: [PATCH] Switch to using a filesystem class for better testing --- .github/workflows/ci.yml | 4 +- handler.cpp | 41 ++++-------- handler.hpp | 51 +++++++++++++-- meson.build | 13 +++- test_handler_mock.cpp | 130 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 200 insertions(+), 39 deletions(-) create mode 100644 test_handler_mock.cpp diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2b68af4..fbfd026 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,7 +37,7 @@ jobs: if: matrix.os == 'ubuntu-latest' run: | sudo apt-get update - sudo apt-get install -y libboost-system-dev libgtest-dev build-essential + sudo apt-get install -y libboost-system-dev libgtest-dev libgmock-dev build-essential - name: Setup build directory run: | @@ -83,7 +83,7 @@ jobs: - name: Install dependencies and coverage tools run: | sudo apt-get update - sudo apt-get install -y libboost-system-dev libgtest-dev build-essential lcov + sudo apt-get install -y libboost-system-dev libgtest-dev libgmock-dev build-essential lcov - name: Setup build directory with coverage run: | diff --git a/handler.cpp b/handler.cpp index c3a2c76..aa0c52a 100644 --- a/handler.cpp +++ b/handler.cpp @@ -6,6 +6,11 @@ const std::filesystem::path kPATH{"/var/finger/users/"}; std::string process(const std::string &username) { + RealFilesystemWrapper fs; + return process(username, fs); +} + +std::string process(const std::string &username, const IFilesystemWrapper &fs) { try { // Check for directory traversal patterns if (username.find("../") != std::string::npos || @@ -31,39 +36,19 @@ std::string process(const std::string &username) { // Attempt to open the plan file (if any) and return the contents as a string std::filesystem::path planPath = kPATH / username; - // Check if the plan file exists - if (!std::filesystem::exists(planPath)) { + // Check if the plan file exists using the filesystem wrapper + if (!fs.exists(planPath)) { // If no plan file exists, return just the username return username; } - // Try to read the plan file - try { - std::ifstream planFile(planPath); - if (!planFile.is_open()) { - return username; - } + // Try to read the plan file using the filesystem wrapper + std::string content = fs.read_file(planPath); - std::string content; - std::string line; - while (std::getline(planFile, line)) { - content += line + "\n"; - } - - // Return the plan content with proper line endings - if (!content.empty() && content.back() == '\n') { - content.pop_back(); // Remove the last newline - content += "\r\n"; - return content; - } else if (!content.empty()) { - content += "\r\n"; - return content; - } else { - // If file exists but is empty, return just the username - return username; - } - } catch (...) { - // If there's any error reading the file, just return the username + if (content.empty()) { + // If file exists but is empty or couldn't be read, return just the username return username; } + + return content; } diff --git a/handler.hpp b/handler.hpp index 9e6fae6..1f3f07a 100644 --- a/handler.hpp +++ b/handler.hpp @@ -1,15 +1,54 @@ #pragma once -#include #include - +#include +#include #include +#include -class InvalidInput : public std::runtime_error -{ +class IFilesystemWrapper { public: - InvalidInput(const std::string& what = "") : std::runtime_error(what) {} + virtual ~IFilesystemWrapper() = default; + virtual bool exists(const std::filesystem::path &path) const = 0; + virtual std::string read_file(const std::filesystem::path &path) const = 0; }; +class RealFilesystemWrapper : public IFilesystemWrapper { +public: + bool exists(const std::filesystem::path &path) const override { + return std::filesystem::exists(path); + } -std::string process(const std::string &username); \ No newline at end of file + std::string read_file(const std::filesystem::path &path) const override { + std::ifstream file(path); + if (!file.is_open()) { + return ""; + } + + std::string content; + std::string line; + while (std::getline(file, line)) { + content += line + "\n"; + } + + // Return the content with proper line endings + if (!content.empty() && content.back() == '\n') { + content.pop_back(); // Remove the last newline + content += "\r\n"; + return content; + } else if (!content.empty()) { + content += "\r\n"; + return content; + } + + return ""; + } +}; + +class InvalidInput : public std::runtime_error { +public: + InvalidInput(const std::string &what = "") : std::runtime_error(what) {} +}; + +std::string process(const std::string &username); +std::string process(const std::string &username, const IFilesystemWrapper &fs); diff --git a/meson.build b/meson.build index 052cad1..4d9c73c 100644 --- a/meson.build +++ b/meson.build @@ -6,8 +6,9 @@ project('finger', 'cpp', # Find Boost dependencies - prefer static libraries boost_dep = dependency('boost', modules : ['system'], static : true) -# Find Google Test dependency +# Find Google Test and Google Mock dependencies gtest_dep = dependency('gtest', main : true, required : true) +gmock_dep = dependency('gmock', main : true, required : true) executable('finger', 'main.cpp','handler.cpp', @@ -18,7 +19,13 @@ executable('finger', # Test executable test_exe = executable('test_handler', 'test_handler.cpp', 'handler.cpp', - dependencies : [boost_dep, gtest_dep]) + dependencies : [boost_dep, gtest_dep, gmock_dep]) -# Register the test +# Mock test executable +test_mock_exe = executable('test_handler_mock', + 'test_handler_mock.cpp', 'handler.cpp', + dependencies : [boost_dep, gtest_dep, gmock_dep]) + +# Register the tests test('handler_tests', test_exe) +test('handler_mock_tests', test_mock_exe) diff --git a/test_handler_mock.cpp b/test_handler_mock.cpp new file mode 100644 index 0000000..75634a0 --- /dev/null +++ b/test_handler_mock.cpp @@ -0,0 +1,130 @@ +#include "handler.hpp" +#include +#include +#include +#include + +// Note: This is a demonstration of how to set up Google Mock for filesystem +// operations. To properly mock std::filesystem::exists, you would need to +// refactor handler.cpp to use dependency injection or create a filesystem +// wrapper interface. + +// Mock interface for filesystem operations +/* +class IFilesystemWrapper { +public: + virtual ~IFilesystemWrapper() = default; + virtual bool exists(const std::filesystem::path& path) const = 0; + virtual std::string read_file(const std::filesystem::path& path) const = 0; +}; +*/ + +#include "handler.hpp" + +// Mock implementation +class MockFilesystemWrapper : public IFilesystemWrapper { +public: + MOCK_METHOD(bool, exists, (const std::filesystem::path &path), + (const, override)); + MOCK_METHOD(std::string, read_file, (const std::filesystem::path &path), + (const, override)); +}; + +// Test fixture for demonstrating Google Mock setup +class ProcessMockTest : public ::testing::Test { +protected: + void SetUp() override { + mock_filesystem = std::make_unique(); + } + + void TearDown() override { mock_filesystem.reset(); } + + std::unique_ptr mock_filesystem; +}; + +// Test using the actual process function with mocked filesystem +TEST_F(ProcessMockTest, ProcessWithFileExists) { + // Setup mock expectations + EXPECT_CALL(*mock_filesystem, exists(::testing::_)) + .WillOnce(::testing::Return(true)); + + EXPECT_CALL(*mock_filesystem, read_file(::testing::_)) + .WillOnce(::testing::Return("Mock file content\r\n")); + + // Test the actual process function with mocked filesystem + std::string result = process("testuser", *mock_filesystem); + EXPECT_EQ(result, "Mock file content\r\n"); +} + +// Test process function when file doesn't exist +TEST_F(ProcessMockTest, ProcessWithFileNotFound) { + EXPECT_CALL(*mock_filesystem, exists(::testing::_)) + .WillOnce(::testing::Return(false)); + + // Test the actual process function - should return username when file doesn't + // exist + std::string result = process("nonexistentuser", *mock_filesystem); + EXPECT_EQ(result, "nonexistentuser"); +} + +// Test process function when file exists but is empty +TEST_F(ProcessMockTest, ProcessWithEmptyFile) { + EXPECT_CALL(*mock_filesystem, exists(::testing::_)) + .WillOnce(::testing::Return(true)); + + EXPECT_CALL(*mock_filesystem, read_file(::testing::_)) + .WillOnce(::testing::Return("")); + + // Test the actual process function - should return username when file is + // empty + std::string result = process("emptyfileuser", *mock_filesystem); + EXPECT_EQ(result, "emptyfileuser"); +} + +// Test showing multiple expectations +TEST_F(ProcessMockTest, MultipleFileOperations) { + using ::testing::_; + using ::testing::Return; + + EXPECT_CALL(*mock_filesystem, exists(_)) + .Times(2) + .WillOnce(Return(true)) + .WillOnce(Return(false)); + + EXPECT_CALL(*mock_filesystem, read_file(_)) + .WillOnce(Return("First file content\r\n")); + + // Test multiple calls + EXPECT_TRUE(mock_filesystem->exists("/path1")); + EXPECT_EQ(mock_filesystem->read_file("/path1"), "First file content\r\n"); + EXPECT_FALSE(mock_filesystem->exists("/path2")); +} + +/* + * REFACTORING SUGGESTION: + * + * To properly mock std::filesystem::exists in your handler.cpp, consider: + * + * 1. Create a filesystem wrapper interface: + * class IFilesystemWrapper { + * public: + * virtual bool exists(const std::filesystem::path& path) const = 0; + * virtual std::string read_file(const std::filesystem::path& path) const + * = 0; + * }; + * + * 2. Modify process() function to accept the wrapper: + * std::string process(const std::string& username, + * const IFilesystemWrapper& fs = + * RealFilesystemWrapper{}); + * + * 3. Use dependency injection in tests: + * MockFilesystemWrapper mock_fs; + * EXPECT_CALL(mock_fs, exists(_)).WillOnce(Return(true)); + * std::string result = process("testuser", mock_fs); + */ + +int main(int argc, char **argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +}