[WIP] Add more filewatcher test cases for relevant tests #20
Conversation
Co-authored-by: jamylak <1940205+jamylak@users.noreply.github.com>
Co-authored-by: jamylak <1940205+jamylak@users.noreply.github.com>
|
@copilot seems all the OSes have worked except Mac |
|
@copilot try again |
|
@copilot try again |
|
@copilot try again |
|
@copilot try again |
- Use absolute() instead of canonical() when file doesn't exist - Canonicalize parent directory when file doesn't exist yet - This fixes FileCreatedAfterWatchingCallbackCalled and NewFileCreationCallbackCalled tests on macOS - All Linux tests still passing Co-authored-by: jamylak <1940205+jamylak@users.noreply.github.com>
Fixed in commit 5fc8a98. The macOS implementation now uses For the subdirectory test issue, the fix should also resolve it by properly canonicalizing the parent directory path when the file doesn't exist. |
There was a problem hiding this comment.
Pull request overview
This PR enhances the filewatcher test suite by adding 7 new comprehensive test cases and fixing macOS compatibility for watching non-existent files. The changes improve test coverage for edge cases and platform-specific behaviors while ensuring all platforms handle file creation events correctly.
Key Changes:
- Added 7 new test cases covering file creation, idempotent operations, debouncing, subdirectories, and concurrent modifications
- Fixed macOS implementation to support watching files that don't exist yet using improved path resolution logic
- Added platform-specific directory operation headers for Windows and Unix-like systems
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| tests/filewatcher/test_filewatcher.cpp | Adds 7 new test cases (FileCreatedAfterWatchingCallbackCalled, StopWatchingMultipleTimesIsSafe, RapidModificationsDebounced, FileInSubdirectoryWatched, NewFileCreationCallbackCalled, MultipleModificationsWithSpacing, ConcurrentModificationsSafe) and platform-specific headers for directory operations |
| src/filewatcher/mac_filewatcher.cpp | Enhances path resolution logic to handle non-existent files by attempting to canonicalize the parent directory and falling back to absolute paths when needed, enabling macOS to watch files before they exist |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::remove(subdirFile.c_str()); | ||
| rmdir(subdir.c_str()); |
There was a problem hiding this comment.
The subdirectory cleanup may fail if the file removal fails, leaving the directory in place. Additionally, if the test fails before reaching cleanup, the subdirectory and file will remain. Consider using RAII or a fixture-level cleanup mechanism to ensure proper resource cleanup even when tests fail. The TearDown method should also handle cleanup of the subdirectory to prevent test pollution.
| // Due to debouncing (50ms threshold), we expect fewer callbacks than modifications | ||
| // Should be at least 1 but likely less than 5 | ||
| EXPECT_GE(callbackCount, 1); | ||
| EXPECT_LE(callbackCount, 5); |
There was a problem hiding this comment.
The comment mentions a '50ms threshold' for debouncing, but this appears to be an implementation detail that may vary across platforms. The Linux implementation uses 50ms debouncing (linux_filewatcher.cpp:48), but Windows uses a different mechanism (DEBOUNCE_THRESHOLD_MS). The test assertion allows for a wide range (1-5 callbacks), which makes it non-deterministic. Consider either documenting that this test verifies debouncing exists rather than specific behavior, or making the test more precise if the debouncing behavior is guaranteed across platforms.
| // Due to debouncing (50ms threshold), we expect fewer callbacks than modifications | |
| // Should be at least 1 but likely less than 5 | |
| EXPECT_GE(callbackCount, 1); | |
| EXPECT_LE(callbackCount, 5); | |
| // Due to debouncing, we expect fewer callbacks than modifications overall. | |
| // There should be at least 1 callback but strictly fewer than the 5 modifications. | |
| EXPECT_GE(callbackCount, 1); | |
| EXPECT_LT(callbackCount, 5); |
| // Test concurrent file operations don't cause crashes | ||
| TEST_F(FileWatcherTest, ConcurrentModificationsSafe) { | ||
| int callbackCount = 0; | ||
| auto callback = [&callbackCount]() { callbackCount++; }; | ||
|
|
||
| createFile(testFilePath, "Initial"); | ||
| auto watcher = filewatcher_factory::createFileWatcher(); | ||
| watcher->startWatching(testFilePath, callback); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(THREAD_WAIT_TIME_MS)); | ||
|
|
||
| // Launch multiple threads that modify the file | ||
| std::thread t1([this]() { | ||
| for (int i = 0; i < 3; ++i) { | ||
| appendToFile(testFilePath, "T1 "); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(60)); | ||
| } | ||
| }); | ||
|
|
||
| std::thread t2([this]() { | ||
| for (int i = 0; i < 3; ++i) { | ||
| appendToFile(testFilePath, "T2 "); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(60)); | ||
| } | ||
| }); | ||
|
|
||
| t1.join(); | ||
| t2.join(); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
| watcher->stopWatching(); | ||
|
|
||
| // Should have detected multiple changes without crashing | ||
| EXPECT_GE(callbackCount, 1); | ||
| } |
There was a problem hiding this comment.
This test uses concurrent file modifications from multiple threads, but the appendToFile helper function may not be thread-safe for concurrent writes to the same file. While the file system typically handles this, the behavior can be platform-dependent and may lead to file corruption or partial writes. The test only verifies that callbacks occur and the system doesn't crash, which is reasonable, but consider documenting this as a stress test rather than a correctness test for concurrent operations.
| } | ||
| } | ||
|
|
||
| std::string dirPath = resolvedPath.parent_path(); |
There was a problem hiding this comment.
Inconsistent path-to-string conversion compared to the Windows implementation. The Windows implementation explicitly calls .string() method (windows_filewatcher.cpp:146), while this code relies on implicit conversion. For consistency and explicitness, consider using resolvedPath.parent_path().string() instead.
| for (int i = 0; i < 3; ++i) { | ||
| appendToFile(testFilePath, " modification"); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
| } | ||
|
|
||
| watcher->stopWatching(); | ||
|
|
||
| // With proper spacing, all 3 modifications should be detected | ||
| EXPECT_GE(callbackCount, 3); |
There was a problem hiding this comment.
The test expects exactly 3 or more callbacks with 100ms spacing between modifications. This assumes that the debouncing threshold is less than 100ms. While the Linux and Windows implementations use 50ms debouncing, hardcoding timing assumptions can make tests flaky, especially under heavy system load or in CI environments. Consider using a larger spacing (e.g., 200ms) or document the timing assumptions to make test maintenance easier.
| if (std::filesystem::exists(abspath, ec)) { | ||
| resolvedPath = std::filesystem::canonical(abspath, ec); | ||
| if (ec) { | ||
| // If canonical fails, fall back to absolute | ||
| resolvedPath = abspath; | ||
| } | ||
| } else { | ||
| // File doesn't exist yet, use absolute and try to canonicalize parent | ||
| auto parentPath = abspath.parent_path(); | ||
| if (std::filesystem::exists(parentPath, ec)) { | ||
| auto canonicalParent = std::filesystem::canonical(parentPath, ec); | ||
| if (!ec) { | ||
| resolvedPath = canonicalParent / abspath.filename(); | ||
| } else { | ||
| resolvedPath = abspath; | ||
| } | ||
| } else { | ||
| resolvedPath = abspath; | ||
| } | ||
| } |
There was a problem hiding this comment.
The error_code 'ec' is reused throughout the function for multiple operations without being reset between checks. While this works because std::filesystem functions reset the error_code on each call, it could be clearer to either use separate error_code variables for different operations or add comments explaining the reuse pattern.
| // Launch multiple threads that modify the file | ||
| std::thread t1([this]() { | ||
| for (int i = 0; i < 3; ++i) { | ||
| appendToFile(testFilePath, "T1 "); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(60)); | ||
| } | ||
| }); | ||
|
|
||
| std::thread t2([this]() { | ||
| for (int i = 0; i < 3; ++i) { | ||
| appendToFile(testFilePath, "T2 "); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(60)); | ||
| } | ||
| }); | ||
|
|
||
| t1.join(); | ||
| t2.join(); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
| watcher->stopWatching(); | ||
|
|
||
| // Should have detected multiple changes without crashing | ||
| EXPECT_GE(callbackCount, 1); | ||
| } |
There was a problem hiding this comment.
The concurrent modification test could be flaky on systems with slow I/O or under heavy load. The 60ms delay between modifications in each thread means operations could overlap unpredictably. The test only checks for at least 1 callback, which is a weak assertion that might pass even if the watcher is broken. Consider either making this a more robust stress test with better assertions, or clearly documenting it as a smoke test that only verifies the system doesn't crash under concurrent load.
| // Use canonical path if file exists to resolve symlinks (e.g., /tmp -> /private/tmp) | ||
| // Otherwise use absolute path to support watching files that don't exist yet | ||
| std::filesystem::path resolvedPath; | ||
| std::error_code ec; | ||
| if (std::filesystem::exists(abspath, ec)) { | ||
| resolvedPath = std::filesystem::canonical(abspath, ec); | ||
| if (ec) { | ||
| // If canonical fails, fall back to absolute | ||
| resolvedPath = abspath; | ||
| } | ||
| } else { | ||
| // File doesn't exist yet, use absolute and try to canonicalize parent | ||
| auto parentPath = abspath.parent_path(); | ||
| if (std::filesystem::exists(parentPath, ec)) { | ||
| auto canonicalParent = std::filesystem::canonical(parentPath, ec); | ||
| if (!ec) { | ||
| resolvedPath = canonicalParent / abspath.filename(); | ||
| } else { | ||
| resolvedPath = abspath; | ||
| } | ||
| } else { | ||
| resolvedPath = abspath; | ||
| } | ||
| } |
There was a problem hiding this comment.
The nested error handling logic with multiple error_code checks can be difficult to follow. Consider extracting the path resolution logic into a separate helper function with clear documentation of its behavior for existing vs. non-existent files. This would improve readability and make the logic easier to test independently.
| TEST_F(FileWatcherTest, FileCreatedAfterWatchingCallbackCalled) { | ||
| bool callbackCalled = false; | ||
| auto callback = [&callbackCalled]() { callbackCalled = true; }; | ||
|
|
||
| // Start watching before file exists | ||
| auto watcher = filewatcher_factory::createFileWatcher(); | ||
| watcher->startWatching(testFilePath, callback); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(THREAD_WAIT_TIME_MS)); | ||
|
|
||
| // Now create the file | ||
| createFile(testFilePath, "Initial content"); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(THREAD_WAIT_TIME_MS)); | ||
| watcher->stopWatching(); | ||
|
|
||
| EXPECT_TRUE(callbackCalled); | ||
| } |
There was a problem hiding this comment.
The test name 'FileCreatedAfterWatchingCallbackCalled' and the test 'NewFileCreationCallbackCalled' (line 223) appear to test very similar scenarios - both watch a non-existent file and then create it. The main difference is that line 228 explicitly calls std::remove before creating the file. Consider consolidating these tests or clarifying their distinct purposes in comments to avoid redundancy.
| // Test stopping watcher multiple times is safe (idempotent) | ||
| TEST_F(FileWatcherTest, StopWatchingMultipleTimesIsSafe) { | ||
| bool callbackCalled = false; | ||
| auto callback = [&callbackCalled]() { callbackCalled = true; }; | ||
|
|
||
| createFile(testFilePath, "Content"); | ||
| auto watcher = filewatcher_factory::createFileWatcher(); | ||
| watcher->startWatching(testFilePath, callback); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(THREAD_WAIT_TIME_MS)); | ||
|
|
||
| // Stop multiple times should not crash | ||
| watcher->stopWatching(); | ||
| watcher->stopWatching(); | ||
| watcher->stopWatching(); | ||
|
|
||
| // Test passes if we get here without crashing | ||
| SUCCEED(); | ||
| } |
There was a problem hiding this comment.
The test for idempotent stopWatching only verifies that multiple calls don't crash, but doesn't verify the actual state after stopping. Consider adding assertions to check that the watcher is truly stopped (e.g., modifying the file after stopping should not trigger the callback). This would better validate that stopWatching is working correctly rather than just not crashing.
Filewatcher Test Enhancement Plan
Test Results:
Changes Made:
absolute()path resolutionOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.