Refactoring for SOLID, DRY, KISS, and Internal Visibility#11
Refactoring for SOLID, DRY, KISS, and Internal Visibility#11
Conversation
… data stores, unit tests, and project configurations including code style.
…ory, lifecycle management, and testing attributes.
…h history tracking and NUnit context integration.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the codebase to align with modern software design principles, focusing on modularity, maintainability, and testability. By separating concerns, centralizing common functionalities like logging and error handling, and updating to the latest framework versions and best practices, the changes enhance the overall quality and future-proofing of the project. The updates also streamline the development experience through consistent code styling and improved NuGet package structure. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring that greatly improves code quality, adheres to SOLID principles, and modernizes the codebase. The extraction of FactoryHistoryManager and TestStoreLifecycleManager is an excellent application of the Single Responsibility Principle. Centralizing exception handling and consistently applying structured logging are also major wins for maintainability and observability. I appreciate the effort to tighten the public API surface by making components internal. My review includes a few minor suggestions to further enhance code clarity and consistency.
| catch (InvalidOperationException) | ||
| { | ||
| throw; | ||
| } |
There was a problem hiding this comment.
This catch block for InvalidOperationException is redundant. An InvalidOperationException thrown within the try block would be caught and immediately re-thrown, which has the same effect as not catching it at all. You can remove this block to simplify the code.
| LogAndCollect(exceptionAggregator, | ||
| $"Failed to delete data for {factoryType} with id {data.Id} and arguments {data.Args}"); |
There was a problem hiding this comment.
The message passed to LogAndCollect is created using string interpolation, which is inconsistent with the PR's goal of using structured logging. The LogAndCollect helper method then logs this pre-formatted string, losing the benefits of structured logging for these specific error cases. Consider handling the logging directly at the call site to use structured logging properly. This would apply to the other LogAndCollect calls in this method as well.
| using DataPreparation.Exceptions; | ||
| using DataPreparation.Factory.Testing; | ||
| using DataPreparation.Helpers; | ||
| using DataPreparation.Helpers; |
There was a problem hiding this comment.
There is a duplicate using DataPreparation.Helpers; statement. One of them can be removed.
There was a problem hiding this comment.
Pull request overview
This PR refactors the DataPreparation library to better separate responsibilities (SRP), centralize exception aggregation, and modernize build/test configuration while tightening the public surface area (marking more components internal) and improving structured logging.
Changes:
- Extracted lifecycle/history responsibilities into
TestStoreLifecycleManagerandFactoryHistoryManager, simplifyingTestStoreandSourceFactory. - Standardized structured logging and centralized cleanup exception aggregation (
ExceptionAggregator.TryExecute*). - Modernized packaging/build: multi-targeting (.NET 8/9/10), SourceLink/symbol packages, repo-wide build props, and added
.editorconfig/CHANGELOG.md.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Directory.Build.props | Adds repo-wide compiler/analyzer defaults. |
| DataPreparation/Stores/TestStoreLifecycleManager.cs | New SRP lifecycle manager for creating/disposing TestStore. |
| DataPreparation/Stores/TestStore.cs | Converts TestStore into a data container; lifecycle moved out. |
| DataPreparation/Stores/HistoryStore.cs | Narrows visibility of HistoryItem<T>. |
| DataPreparation/Stores/DataRelationStore.cs | Removes BOM and an unused static using. |
| DataPreparation/PreparationContext.cs | Updates lookups to use the new lifecycle manager. |
| DataPreparation/Models/Testing/TestInfo.cs | Narrows visibility to internal. |
| DataPreparation/Models/Testing/FixtureInfo.cs | Narrows visibility to internal. |
| DataPreparation/Models/Testing/ContextTestInfo.cs | Narrows visibility to internal. |
| DataPreparation/Models/Testing/AttributeUsingCounter.cs | Fixes typo in method name. |
| DataPreparation/Models/FactoryObjects/Data/FactoryData.cs | Narrows visibility to internal. |
| DataPreparation/Models/Data/ThreadSafeCounter.cs | Narrows visibility to internal. |
| DataPreparation/Interfaces/Testing/Factory/IFactoryData.cs | Narrows visibility to internal. |
| DataPreparation/Helpers/LoggerHelper.cs | Simplifies logger creation and adds doc comments. |
| DataPreparation/Exceptions/ExceptionAggregator.cs | Adds TryExecute* and ThrowIfAny helpers. |
| DataPreparation/Database/Helpers/DockerHelper.cs | Moves Console logging to structured ILogger. |
| DataPreparation/Database/Executer.cs | Removes empty placeholder class. |
| DataPreparation/DataPreparationWorker/SourceFactory.cs | Refactors and delegates history tracking to FactoryHistoryManager. |
| DataPreparation/DataPreparationWorker/GetDataPreparation.cs | Refactors prepared-data resolution and logging/error handling. |
| DataPreparation/DataPreparationWorker/FactoryHistoryManager.cs | New SRP component for tracking/disposing factory-created data. |
| DataPreparation/DataPreparationWorker/DataPreparationHandler.cs | Uses structured logging and fixes exception aggregation flow. |
| DataPreparation/DataPreparation.csproj | Multi-targets net8/9/10; updates package metadata/deps; adds SourceLink/symbol config. |
| DataPreparation/Attributes/Testing/UsePreparedDataParamsForAttribute.cs | Switches to new lifecycle manager APIs. |
| DataPreparation/Attributes/Testing/UsePreparedDataParamsAttribute.cs | Switches to new lifecycle manager APIs. |
| DataPreparation/Attributes/Testing/UsePreparedDataForAttribute.cs | Switches to new lifecycle manager APIs. |
| DataPreparation/Attributes/Testing/UsePreparedDataAttribute.cs | Switches to new lifecycle manager APIs. |
| DataPreparation/Attributes/Testing/DataPreparationTestAttribute.cs | Switches to new lifecycle manager APIs. |
| DataPreparation.UnitTests/Stores/FixtureStoreTests.cs | Disposes service provider in [TearDown]. |
| DataPreparation.UnitTests/DataPreparation.UnitTests.csproj | Updates target framework and test package versions. |
| CHANGELOG.md | Adds changelog for v2.0.0 changes. |
| .editorconfig | Adds formatting/code-style defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (useClassDataPreparation) | ||
| { | ||
| return dataProviders.Select(dataPreparationType => GetPreparedData(testStore, dataPreparationType)).ToList(); | ||
| var classDataType = DataRelationStore.GetClassDataPreparationType(classType) | ||
| ?? throw ThrowNotRegistered(logger, $"Prepared data for class {classType.FullName} not registered."); | ||
| preparedDataTypes.Add(classDataType); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets a single prepared data instance from the service provider by its type. | ||
| /// </summary> | ||
| /// <param name="testStore">The test store containing the service provider and logger factory</param> | ||
| /// <param name="preparedDataType">The type of data preparation class to resolve</param> | ||
| /// <returns>An instantiated data preparation object</returns> | ||
| /// <exception cref="InvalidOperationException"> | ||
| /// Thrown when the data preparation type cannot be resolved from the dependency injection container, | ||
| /// either because it's not registered or because its dependencies cannot be satisfied | ||
| /// </exception> | ||
| /// <remarks> | ||
| /// This method uses dependency injection to create instances of data preparation classes, | ||
| /// allowing those classes to have their own dependencies injected (like database connections). | ||
| /// </remarks> | ||
| private static object GetPreparedData(TestStore testStore, Type preparedDataType) | ||
| else | ||
| { | ||
| var logger = testStore.LoggerFactory.CreateLogger(typeof(GetDataPreparation)); | ||
| logger.LogTrace($"Getting prepared data with type {preparedDataType.FullName}."); | ||
|
|
||
| try | ||
| { | ||
| // Attempt to resolve the data preparation type from the DI container | ||
| var preparedData = testStore.ServiceProvider.GetService(preparedDataType); | ||
|
|
||
| if (preparedData == null) | ||
| { | ||
| logger.LogError($"Prepared data with type {preparedDataType.FullName} not found."); | ||
| throw new InvalidOperationException( | ||
| $"Prepared data with type {preparedDataType.FullName} not found."); | ||
| } | ||
|
|
||
| logger.LogDebug($"Prepared data with type {preparedDataType.FullName} found."); | ||
| return preparedData; | ||
| } | ||
| catch (InvalidOperationException e) | ||
| { | ||
| // This typically happens when the DI container can't satisfy the constructor dependencies | ||
| logger.LogError(e, $"Prepared data with type {preparedDataType.FullName} not found."); | ||
| throw new InvalidOperationException( | ||
| $"For prepared data with type {preparedDataType.FullName} not found suitable constructor, check Dependency Injection.", e); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| // Catch and rethrow any other exception that might occur during resolution | ||
| logger.LogError(e, $"Prepared data with type {preparedDataType.FullName} not found."); | ||
| throw new Exception( | ||
| $"Prepared data with type {preparedDataType.FullName} not found.", e); | ||
| } | ||
| logger.LogTrace("Class data preparation not used for class {ClassType}", classType.FullName); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets prepared data instances by looking up data preparation classes associated with a specified | ||
| /// class and/or methods in the DataRelationStore. | ||
| /// </summary> | ||
| /// <param name="testStore">The test store containing the service provider</param> | ||
| /// <param name="useClassDataPreparation">Whether to use class-level data preparation</param> | ||
| /// <param name="classType">The class type to get associated data preparation for</param> | ||
| /// <param name="methodsNames">Names of methods to get associated data preparation for</param> | ||
| /// <returns>A list of instantiated data preparation objects</returns> | ||
| /// <exception cref="InvalidOperationException"> | ||
| /// Thrown when the specified class or methods don't have registered data preparation types, | ||
| /// or when the methods cannot be found in the class | ||
| /// </exception> | ||
| /// <remarks> | ||
| /// This method enables convention-based data preparation, where data preparation classes can be | ||
| /// associated with test classes and methods using PreparationClassFor and PreparationMethodFor attributes. | ||
| /// This allows for reusing data preparation classes across multiple test methods. | ||
| /// </remarks> | ||
| internal static List<object> GetPreparedDataFromCode(TestStore testStore, bool useClassDataPreparation, Type classType, | ||
| string[] methodsNames) | ||
|
|
||
| foreach (var methodName in methodsNames) | ||
| { | ||
| List<Type?> preparedDataTypes = new(); | ||
| var logger = testStore.LoggerFactory.CreateLogger(typeof(GetDataPreparation)); | ||
|
|
||
| // If using class data preparation, look up the data preparation type for this class | ||
| if (useClassDataPreparation) | ||
| { | ||
| Type? preparedDataType = DataRelationStore.GetClassDataPreparationType(classType); | ||
| if (preparedDataType == null) | ||
| { | ||
| var e = new InvalidOperationException( | ||
| $"Prepared data for class {classType.FullName} not registered."); | ||
| logger.LogError(e, $"Data preparation failed."); | ||
| throw e; | ||
| } | ||
|
|
||
| preparedDataTypes.Add(preparedDataType); | ||
| } | ||
| else | ||
| { | ||
| logger.LogTrace($"Class data preparation not used for class {classType.FullName}."); | ||
| } | ||
| var methodInfo = classType.GetMethod(methodName) | ||
| ?? throw ThrowNotRegistered(logger, $"Method {methodName} not found in class {classType.FullName}."); | ||
|
|
||
| // For each specified method, look up its data preparation type | ||
| foreach (var methodName in methodsNames) | ||
| { | ||
| // Find the method by name | ||
| MethodInfo? methodInfo = classType.GetMethod(methodName); | ||
| if (methodInfo == null) | ||
| { | ||
| var e = new InvalidOperationException( | ||
| $"Method {methodName} not found in class {classType.FullName}."); | ||
| logger.LogError(e, $"Data preparation failed."); | ||
| throw e; | ||
| } | ||
|
|
||
| // Look up data preparation type for this method | ||
| Type? preparedDataType = DataRelationStore.GetMethodDataPreparationType(methodInfo); | ||
| if (preparedDataType == null) | ||
| { | ||
| var e = new InvalidOperationException( | ||
| $"Prepared data for method {methodInfo} not registered."); | ||
| logger.LogError(e, $"Data preparation failed."); | ||
| throw e; | ||
| } | ||
|
|
||
| preparedDataTypes.Add(preparedDataType); | ||
| } | ||
|
|
||
| // Resolve all the identified data preparation types | ||
| return GetPreparedData(testStore, preparedDataTypes.ToArray()); | ||
| var methodDataType = DataRelationStore.GetMethodDataPreparationType(methodInfo) | ||
| ?? throw ThrowNotRegistered(logger, $"Prepared data for method {methodInfo} not registered."); | ||
| preparedDataTypes.Add(methodDataType); |
There was a problem hiding this comment.
ThrowNotRegistered always throws, but it’s being used in expressions like ?? throw ThrowNotRegistered(...). Because the helper throws before returning, the outer throw is never reached, which is confusing and makes the null-coalescing pattern misleading. Consider changing the helper to return an InvalidOperationException (and let the caller throw it), or call a Throw... helper directly without wrapping it in throw expressions.
| if (_factoryHistory.TryGetValue(typeof(TDataFactory), out var history) && | ||
| history.TryGetLatest(out var item, out createdId)) | ||
| { | ||
| logger.LogDebug("[{Operation}]: Retrieved data for {FactoryType} with id {Id}", | ||
| "Get", typeof(TDataFactory), createdId); | ||
|
|
||
| if ((item as FactoryData)!.Data is not TRet data) | ||
| { | ||
| throw CastException($"Data is not of type {typeof(TRet)} for operation Get."); | ||
| } | ||
|
|
||
| ret = data; | ||
| return true; |
There was a problem hiding this comment.
TryGetLatest unnecessarily downcasts item to FactoryData via (item as FactoryData)! and then reads .Data. Since the history store is already typed as IFactoryData, this can be accessed directly, and the current code risks a null reference if a different IFactoryData implementation ever gets stored. Prefer using the interface (item.Data) and avoid the null-forgiving cast.
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="DockerHelper"/> class. | ||
| /// </summary> | ||
| /// <param name="containerId">The Docker container ID</param> | ||
| /// <param name="databaseName">The name of the PostgreSQL database</param> | ||
| /// <param name="userName">The database user name</param> | ||
| /// <param name="password">The database password</param> | ||
| /// <param name="logger">Logger instance for structured logging</param> | ||
| public DockerHelper(string containerId, string databaseName, string userName, string password, ILogger<DockerHelper> logger) | ||
| { | ||
| _containerID = containerId; | ||
| _databaseName = databaseName; | ||
| _userName = userName; | ||
| _password = password; | ||
| _logger = logger; | ||
| _backupFilePath = string.Format(_baseBackupFilePath, _databaseName); |
There was a problem hiding this comment.
This is a breaking change to a public API: the previous DockerHelper(string containerId, string databaseName, string userName, string password) constructor is removed and replaced with one that requires an ILogger<DockerHelper>. If external consumers instantiate this type directly, they’ll now fail to compile. Consider keeping the original overload (e.g., chaining to the new one with NullLogger<DockerHelper>.Instance) or making the logger optional to preserve backwards compatibility.
| using DataPreparation.Helpers; | ||
| using DataPreparation.Testing; | ||
| using DataPreparation.Testing.Factory; | ||
| using Microsoft.Extensions.DependencyInjection; |
There was a problem hiding this comment.
There are redundant/unused using directives here (e.g., using DataPreparation.Helpers; is duplicated, and Microsoft.Extensions.DependencyInjection appears unused in this file). With EnforceCodeStyleInBuild enabled, these can trigger IDE analyzers; please remove the duplicates/unnecessary usings.
| using DataPreparation.Helpers; | |
| using DataPreparation.Testing; | |
| using DataPreparation.Testing.Factory; | |
| using Microsoft.Extensions.DependencyInjection; | |
| using DataPreparation.Testing; | |
| using DataPreparation.Testing.Factory; |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| } | ||
|
|
||
| preparedDataTypes.Add(preparedDataType); | ||
| } |
There was a problem hiding this comment.
ThrowNotRegistered throws instead of returning the exception
Medium Severity
The ThrowNotRegistered helper method declares a return type of InvalidOperationException but uses throw exception; instead of return exception; on line 152. This means the method always throws internally, making the throw keywords at all four call sites (lines 45, 56, 59, 120) dead code. This is inconsistent with the analogous CastException helper in FactoryHistoryManager which correctly returns the exception. The current code works functionally but produces misleading stack traces pointing to the helper method rather than the actual error location.
| { | ||
| try { action(); } | ||
| catch (Exception e) { Add(e); } | ||
| } |
There was a problem hiding this comment.
TryExecute loses AggregateException flattening from old code
Medium Severity
TryExecute catches Exception and calls Add(Exception?), which does not flatten AggregateExceptions. The old Deinitialize code explicitly caught AggregateException and called the Add(AggregateException) overload that flattens inner exceptions. Since DisposeAsync and DataDown can both throw AggregateException, the cleanup path now produces nested AggregateException structures instead of a flat list, degrading error diagnostics.
Additional Locations (1)
| { | ||
| try { await action().ConfigureAwait(false); } | ||
| catch (Exception e) { Add(e); } | ||
| } |
There was a problem hiding this comment.
New TryExecuteAsync method is never called anywhere
Low Severity
The newly added TryExecuteAsync method in ExceptionAggregator is defined but never called anywhere in the codebase. All cleanup operations in TestStoreLifecycleManager.Deinitialize use the synchronous TryExecute with .GetAwaiter().GetResult() instead. This is unused dead code.
| using DataPreparation.Exceptions; | ||
| using DataPreparation.Factory.Testing; | ||
| using DataPreparation.Helpers; | ||
| using DataPreparation.Helpers; |
There was a problem hiding this comment.
Duplicate using directive in TestStoreLifecycleManager
Low Severity
using DataPreparation.Helpers; appears twice on lines 4 and 5 of TestStoreLifecycleManager.cs. This is a redundant duplicate import.
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Overview
Targeting .NET 8/9/10\ with zero compilation errors and all 65 unit tests passing successfully.
Note
Medium Risk
Medium risk due to a breaking NUnit 3→4 upgrade plus refactors to
TestStorelifecycle/cleanup and factory history tracking that could change test data setup/teardown behavior and public API surface viainternalvisibility changes.Overview
Modernizes packaging/build and upgrades dependencies: the library now multi-targets
net8.0;net9.0;net10.0, enables SourceLink/symbol packages, deterministic builds, XML docs, addsDirectory.Build.props,.editorconfig, and aCHANGELOG.md, and bumps version to2.0.0with updatedMicrosoft.Extensions.*packages.Refactors core test lifecycle and factory tracking:
TestStoreis turned into a data container while lifecycle logic moves to newTestStoreLifecycleManager, andSourceFactoryis slimmed down by extracting history/cleanup into newFactoryHistoryManager. Cleanup/error handling is centralized via newExceptionAggregatorhelpers,DataPreparationHandler.DataDownnow aggregates teardown exceptions correctly, and logging is standardized to structuredILoggercalls (notably inDockerHelper).Visibility/API tightening and small removals: several types are made
internal, an emptyDatabase/Executer.csis removed, and an unusedusingis dropped; unit tests and example projects are updated to NUnit 4.5.1 and newer test tooling.Written by Cursor Bugbot for commit cc297aa. This will update automatically on new commits. Configure here.