Skip to content

Adding tests for filewatcher sources#1

Merged
codegefluester merged 3 commits intomainfrom
feat/add-basic-tests-for-filewatcher-sources
Feb 6, 2026
Merged

Adding tests for filewatcher sources#1
codegefluester merged 3 commits intomainfrom
feat/add-basic-tests-for-filewatcher-sources

Conversation

@codegefluester
Copy link
Copy Markdown
Owner

Auto-discovers file watcher sources and tests that they work correctly. We exclude Tekken for now since I do not know what the file extension is yet.

Copilot AI review requested due to automatic review settings February 6, 2026 12:16
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an xUnit-based test suite that auto-discovers FileWatcherSourceBase implementations via reflection and validates key behaviors (startup scan, file create detection, subdirectory watching, debouncing / single-emission).

Changes:

  • Added FileWatcherSourceTests to exercise file-watcher sources against real filesystem events.
  • Added reflection-based discovery helpers (FileWatcherSourceDiscovery) and xUnit MemberData generator (FileWatcherTestData).
  • Introduced basic exclusions (Tekken8) and source grouping by subdirectory support.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.

File Description
GamesDat.Tests/Helpers/FileWatcherTestData.cs Generates xUnit MemberData for discovered file watcher sources (and subdirectory groupings).
GamesDat.Tests/Helpers/FileWatcherSourceDiscovery.cs Reflection-based discovery + pattern/subdirectory extraction + source instantiation helper.
GamesDat.Tests/FileWatcherSourceTests.cs New parameterized tests validating file watcher source behavior on the filesystem.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Address code review feedback from PR #1 with the following improvements:

**Resource Management:**
- Add `using` declarations to all test methods for proper disposal
- Implement try/finally blocks to ensure cleanup even on assertion failures
- Guarantee CancellationTokenSource cancellation and task awaiting in all paths

**Type Safety:**
- Replace string-based type exclusions with typeof() comparisons
- Add explicit Tekken8ReplayFileSource import for compile-time verification

**Pattern Discovery:**
- Support both ApplyDefaults(FileWatcherOptions) and ApplyDefaults(string?) overloads
- Throw InvalidOperationException when patterns cannot be retrieved
- Validate patterns in test data generation (fail-fast on missing configuration)

**Test Coverage:**
- Update AllDiscoveredSources_AreInstantiable to use DiscoverAllSources() directly
- Ensure all discovered sources are validated (no silent exclusions)
- Add explicit pattern validation for all sources except Tekken8

These changes ensure reliable test cleanup, prevent resource leaks, and
guarantee complete test coverage of all FileWatcher source implementations.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@codegefluester
Copy link
Copy Markdown
Owner Author

Review Comment Responses

Thank you for the thorough review! I've addressed 7 of the 9 comments in commit 4801846. Here's a summary:

✅ Addressed Comments

1. Type-Safe Exclusions (FileWatcherTestData.cs:27, 60)

  • Changed from string comparison to typeof(Tekken8ReplayFileSource)
  • Added explicit import for refactor-safety

2. Resource Management (FileWatcherSourceTests.cs:60, 145, 230, 300, 382, 468)

  • Added using var source to all test methods for proper disposal
  • Ensures FileSystemWatcher resources are cleaned up

3. Reliable Cleanup (FileWatcherSourceTests.cs - all async tests)

  • Added try/finally blocks to all test methods
  • Guarantees CTS cancellation and watchTask awaiting even on assertion failures

4. Complete Source Validation (FileWatcherSourceTests.cs:549-576)

  • Changed AllDiscoveredSources_AreInstantiable() to use DiscoverAllSources() directly
  • Validates all sources including explicit pattern checks (no silent exclusions)

5. Comprehensive Pattern Discovery (FileWatcherSourceDiscovery.cs:96-130)

  • GetDefaultOptions() now supports both ApplyDefaults(FileWatcherOptions) and ApplyDefaults(string?)
  • Tries FileWatcherOptions overload first, then falls back to string overload
  • Discovers patterns from all source implementations

6. Fail-Fast Pattern Retrieval (FileWatcherSourceDiscovery.cs:56-68)

  • GetExpectedPatterns() now throws InvalidOperationException instead of returning empty
  • Test failures are explicit and debuggable

7. Pattern Validation in Test Data (FileWatcherTestData.cs:32-38, 71-77)

  • AllSources() and SourcesWithSubdirectories() throw when patterns are empty
  • Ensures test coverage, fails loudly if configuration is incomplete

📝 Not Addressed (Style Suggestions)

8 & 9. Using explicit .Where() instead of continue

I've opted to keep the current continue pattern for the following reasons:

  1. Early Exit Clarity: The continue statements serve as early-exit guards that are immediately visible when reading the loop. This makes the filtering logic explicit and easy to understand at a glance.

  2. Maintainability: With multiple filtering conditions (Tekken8 exclusion, subdirectory check, pattern validation), chaining .Where() clauses would require either:

    • Multiple .Where() calls which is less efficient (multiple enumerations)
    • Complex lambda predicates that are harder to read and debug
    • Variable captures in closures
  3. Debug-Friendly: With continue, each filtering condition can be individually debugged and has its own explanatory comment. The loop body shows exactly what gets yielded.

  4. Common Pattern: This pattern is idiomatic in C# for test data generation methods where conditions need documentation and the filtering logic benefits from being explicit.

If you feel strongly about using .Where(), I'm happy to refactor, but I believe the current approach is more maintainable for this use case.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Add explicit System.IO import to FileWatcherSourceDiscovery
- Fix constructor invocation to use Array.Empty<object>() instead of null
- Add test for sources without subdirectory support

Addresses Copilot PR review comments:
1. Missing namespace import for Path.GetTempPath()
2. Unused SourcesWithoutSubdirectories() test data method
3. Incorrect constructor invocation with null parameter

The new test gracefully handles sources with inconsistent subdirectory
configuration between ApplyDefaults and string constructors.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@codegefluester codegefluester merged commit 61731fc into main Feb 6, 2026
1 check passed
@codegefluester codegefluester deleted the feat/add-basic-tests-for-filewatcher-sources branch February 6, 2026 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants