Skip to content

test: Improve test module return types and document external URL dependency#1673

Merged
thomhurst merged 1 commit into
mainfrom
fix/test-improvements-2
Dec 30, 2025
Merged

test: Improve test module return types and document external URL dependency#1673
thomhurst merged 1 commit into
mainfrom
fix/test-improvements-2

Conversation

@thomhurst
Copy link
Copy Markdown
Owner

Summary

  • Change test modules from Module<IDictionary<string, object>?> to Module<bool> where return value was always null (cleaner and more explicit)
  • Add documentation comment to DownloaderTests explaining the external URL dependency

26 test files modified including:
AfterPipelineLoggerTests, AlwaysRunTests, AsyncDisposableModuleTests, DependsOnTests, DirectCollisionTests, DisposableModuleTests, EngineCancellationTokenTests, FailedPipelineTests, LoggingSecretTests, ModuleHistoryTests, ModuleLoggerTests, ModuleNotRegisteredExceptionTests, NestedCollisionTests, OneWayDependenciesNonCollisionTests, PipelineProgressTests, PipelineRequirementTests, ResultsRepositoryTests, RetryTests, RunnableCategoryTests, SafeEstimatedTimeProviderTests, SkipDependabotAttributeTests, TimedDependencyTests, UnusedModuleDetectorTests

Special cases:

  • JsonSerializationTests: Uses non-nullable IDictionary<string, object> where actual dictionary values are needed
  • FailedModuleWithCustomRetryPolicy: Uses string return type to work with IRetryable<T> interface

Fixes #1597, Fixes #1619

Test plan

  • Build project to verify changes compile
  • Verify test structure remains correct

🤖 Generated with Claude Code

…ndency

- Change test modules from IDictionary<string, object>? to bool when no
  meaningful return value is needed (Issue #1597). Using bool return type
  is cleaner and more explicit than returning null for nullable dictionaries.

- Add documentation comment to DownloaderTests explaining the external URL
  dependency and why this approach is reasonable for testing real download
  functionality (Issue #1619).

- Keep IDictionary<string, object> (non-nullable) for JsonSerializationTests
  where actual dictionary values are needed for testing serialization.

- Use string return type for FailedModuleWithCustomRetryPolicy to work with
  the IRetryable<T> interface (value types have issues with T? generics).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

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

Summary

Refactors test modules to use more appropriate return types: Module<bool> where tests always returned null, and Module<IDictionary<string, object>> (non-nullable) where tests return actual dictionary values.

Critical Issues

None found ✅

Suggestions

None - the changes are clean and improve code clarity. The non-nullable dictionary type in JsonSerializationTests correctly reflects that those modules always return actual dictionary instances, while the bool type for other test modules is more explicit than returning null from a nullable dictionary type.

Previous Review Status

No previous comments.

Verdict

APPROVE - No critical issues

The changes improve test code readability and type safety without affecting functionality. All modified return types correctly reflect the actual behavior of the test modules.

@thomhurst thomhurst merged commit ef8e766 into main Dec 30, 2025
10 of 12 checks passed
@thomhurst thomhurst deleted the fix/test-improvements-2 branch December 30, 2025 15:20
@thomhurst thomhurst restored the fix/test-improvements-2 branch December 30, 2025 15:22
@thomhurst thomhurst deleted the fix/test-improvements-2 branch December 30, 2025 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant