V4.0.0/abstraction improvements #9
Conversation
WalkthroughThe changes update the CI/CD pipeline, solution structure, package versions, and application lifecycle management. The GitHub workflows now use updated OS versions and restructured jobs with a matrix strategy for testing. New projects and configurations have been added to the solution file and test projects. Package references have been updated in the props files. Application code has been refactored to use the new Changes
Sequence Diagram(s)sequenceDiagram
participant Host
participant LT as IHostLifetimeEvents
participant App as Application
Host->>LT: Start Host
LT-->>App: Trigger OnApplicationStartedCallback
App->>LT: Log startup event
Host->>LT: Initiate shutdown
LT-->>App: Trigger OnApplicationStoppingCallback
App->>LT: Perform graceful shutdown
LT-->>Host: Trigger OnApplicationStoppedCallback
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (46)
💤 Files with no reviewable changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Copilot reviewed 38 out of 41 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- Codebelt.Bootstrapper.sln: Language not supported
- Directory.Build.props: Language not supported
- Directory.Packages.props: Language not supported
Comments suppressed due to low confidence (1)
app/Codebelt.Bootstrapper.Worker.App/FakeHostedService.cs:23
- Consider replacing Thread.Sleep with an asynchronous delay (e.g., Task.Delay) to avoid blocking the thread during shutdown.
Thread.Sleep(TimeSpan.FromSeconds(5)); // simulate graceful shutdown
| _events.OnApplicationStartedCallback += () => | ||
| { |
There was a problem hiding this comment.
[nitpick] Consider unsubscribing from the OnApplicationStartedCallback event after the callback has executed to avoid potential memory leaks.
| _events.OnApplicationStartedCallback += () => | |
| { | |
| Action onApplicationStarted = null; | |
| onApplicationStarted = () => | |
| { | |
| _events.OnApplicationStartedCallback -= onApplicationStarted; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (23)
testenvironments.json (1)
1-15: Configuration format looks good, but version should be a semantic versionThe file successfully defines two test environments (WSL-Ubuntu and Docker-Ubuntu) with their respective configurations. This will be useful for testing across different platforms.
However, consider using a semantic version format (e.g., "1.0.0") for the version field instead of just "1" to better follow versioning conventions.
- "version": "1", + "version": "1.0.0",test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/ManualMinimalHostFixture.cs (1)
9-9: Consider adding documentation for empty callback functionThe
HostRunnerCallbackis set to an empty lambda function. While this might be intentional for manual control of the host runner, it's not immediately clear why this is being done.Add XML documentation to explain the purpose of the empty callback and how this fixture is intended to be used.
public ManualMinimalHostFixture() { - HostRunnerCallback = host => { }; + // Empty callback allows tests to manually control host execution + HostRunnerCallback = host => { }; }test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestConsoleStartup.cs (1)
17-19: Consider test service registration.While the empty
ConfigureServicesmethod is fine for basic tests, consider adding test-specific services if needed to verify dependency injection behavior, similar to howTestStartupRootregisters a test service.src/Codebelt.Bootstrapper.Web/MinimalWebProgram.cs (1)
15-15: Fix inconsistent return type in XML documentation.There's an inconsistency in the XML documentation where the return type is still documented as
HostApplicationBuilderwhile the actual method returnsWebApplicationBuilder.- /// <returns>The initialized <see cref="HostApplicationBuilder"/>.</returns> + /// <returns>The initialized <see cref="WebApplicationBuilder"/>.</returns>test/Codebelt.Bootstrapper.Tests/Assets/StartupRootUnsafeAccessor.cs (1)
5-14: Consider adding class-level documentationGood use of the C# 12
UnsafeAccessorattribute to access internal properties ofStartupRootfor testing purposes. This approach is more performant than using reflection.Consider adding XML documentation for the class itself to explain its testing purpose and usage constraints:
namespace Codebelt.Bootstrapper.Assets { + /// <summary> + /// Test utility class that provides access to internal properties of <see cref="StartupRoot"/> + /// using the UnsafeAccessor feature for testing purposes only. + /// </summary> public static class StartupRootUnsafeAccessor { [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "get_Configuration")] public static extern IConfiguration GetConfiguration(StartupRoot startup); [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "get_Environment")] public static extern IHostEnvironment GetEnvironment(StartupRoot startup); } }src/Codebelt.Bootstrapper/HostBuilderExtensions.cs (1)
24-24: Good approach for service registrationThis is a clean way to expose the same
BootstrapperLifetimeinstance through the newIHostLifetimeEventsinterface. The code correctly leverages the fact thatBootstrapperLifetimeimplements both interfaces.For slightly improved robustness, you could consider this alternative that doesn't rely on service ordering:
-services.Replace(ServiceDescriptor.Singleton<IHostLifetime, BootstrapperLifetime>()); -services.AddSingleton<IHostLifetimeEvents>(provider => provider.GetRequiredService<IHostLifetime>() as BootstrapperLifetime); +var lifetime = new ServiceDescriptor(typeof(BootstrapperLifetime), typeof(BootstrapperLifetime), ServiceLifetime.Singleton); +services.Add(lifetime); +services.AddSingleton<IHostLifetime>(p => p.GetRequiredService<BootstrapperLifetime>()); +services.AddSingleton<IHostLifetimeEvents>(p => p.GetRequiredService<BootstrapperLifetime>());However, the current implementation is perfectly reasonable and more concise.
test/Codebelt.Bootstrapper.Console.FunctionalTests/LoggerExtensions.cs (1)
8-25: Consider adding a generic overload for convenienceThe extension method is well-implemented with thorough documentation. This will be helpful for testing log output in a structured way.
Consider adding a generic overload to make the API more fluent when the calling code knows the type at compile time:
namespace Codebelt.Bootstrapper.Console { public static class LoggerExtensions { // Existing implementation... + /// <summary> + /// Returns the associated <see cref="ITestStore{T}"/> that is provided when settings up services from <see cref="ServiceCollectionExtensions.AddXunitTestLogging"/>. + /// </summary> + /// <typeparam name="T">The type for which the logger was created.</typeparam> + /// <param name="logger">The <see cref="ILogger"/> from which to retrieve the <see cref="ITestStore{T}"/>.</param> + /// <returns>Returns an implementation of <see cref="ITestStore{T}"/> with all logged entries expressed as <see cref="XunitTestLoggerEntry"/>.</returns> + /// <exception cref="ArgumentNullException"> + /// <paramref name="logger"/> cannot be null. + /// </exception> + /// <exception cref="ArgumentException"> + /// <paramref name="logger"/> does not contain a test store. + /// </exception> + public static ITestStore<XunitTestLoggerEntry> GetTestStore<T>(this ILogger logger) + { + return GetTestStore(logger, typeof(T)); + } } }src/Codebelt.Bootstrapper/IHostLifetimeEvents.cs (1)
5-24: Interface design is well-structured but deviates from typical .NET event patternsThe interface provides a clean abstraction for application lifecycle events with thorough documentation.
Consider that this interface uses properties of type
Actionrather than standard .NET events. While this approach simplifies direct assignment of callbacks, it:
- Doesn't support multiple subscribers (unlike events)
- Deviates from typical .NET event naming conventions (which would omit "On" prefix and "Callback" suffix)
For future consideration, a more idiomatic .NET approach would be:
public interface IHostLifetimeEvents { /// <summary> /// Triggered when the application host has fully started. /// </summary> event EventHandler ApplicationStarted; /// <summary> /// Triggered when the application host is starting a graceful shutdown. /// </summary> event EventHandler ApplicationStopping; /// <summary> /// Triggered when the application host has completed a graceful shutdown. /// </summary> event EventHandler ApplicationStopped; }That said, the current design works well and is consistent with the existing
BootstrapperLifetimeimplementation.test/Codebelt.Bootstrapper.FunctionalTests/HostApplicationBuilderExtensionsTest.cs (1)
1-14: Remove unused using statementsThere are several unused using statements that should be removed to keep the code clean.
-using System.Collections.Generic; -using System.Linq; -using System.Text; -using System.Threading.Tasks; -using static System.Net.Mime.MediaTypeNames;test/Codebelt.Bootstrapper.FunctionalTests/BootstrapperLifetimeTest.cs (1)
38-60: Good test for application stopping/stopped callbacks.The test properly verifies both stopping and stopped callbacks are invoked during host shutdown. Consider using async/await pattern instead of GetAwaiter().GetResult() for better exception handling.
- test.Host.StopAsync().GetAwaiter().GetResult(); + await test.Host.StopAsync();test/Codebelt.Bootstrapper.FunctionalTests/HostedServiceExtensionsTest.cs (2)
20-49: Test contains an unused variable and extra spacing.The test correctly verifies that the application waits for the "application started" announcement. However, there's an unused variable
startedand unnecessary empty lines.- var timeToWait = TimeSpan.FromMilliseconds(50); - var started = false; + var timeToWait = TimeSpan.FromMilliseconds(50); using var test = HostTestFactory.Create(services => ... test.Host.Services.GetRequiredService<IHostLifetimeEvents>().OnApplicationStartedCallback += () => { Thread.Sleep(timeToWait); }; - var bgs = test.Host.Services.GetRequiredService<IHostedService>() as TestBackgroundService;Also, consider using
GetRequiredService<TestBackgroundService>()directly instead of casting from IHostedService for more robust type safety.
36-40: Consider using async-friendly delay mechanism.Using Thread.Sleep in an asynchronous callback can block threads in the thread pool. Consider using Task.Delay instead, which allows the thread to be returned to the pool during the delay.
test.Host.Services.GetRequiredService<IHostLifetimeEvents>().OnApplicationStartedCallback += () => { - Thread.Sleep(timeToWait); + Task.Delay(timeToWait).Wait(); // Still blocks but signals async intent };For a fully asynchronous approach (which would require restructuring the test):
test.Host.Services.GetRequiredService<IHostLifetimeEvents>().OnApplicationStartedCallback += async () => { await Task.Delay(timeToWait); };test/Codebelt.Bootstrapper.Console.FunctionalTests/MinimalConsoleHostedServiceTest.cs (3)
17-30: Reconsider global exception handling in test constructor.Setting up global exception handlers in a test constructor could potentially interfere with the test framework's own error handling. Consider moving this to a more isolated scope or removing it if not strictly necessary for the test.
If you need to capture exceptions during the test, consider using a more localized approach that doesn't affect the entire AppDomain.
32-44: Test setup looks good but host startup is implicit.The test configures the host correctly, but there's no explicit call to start the host before waiting for shutdown. This may confuse readers about how the host lifecycle is being managed.
await using var test = MinimalHostTestFactory.Create(services => { services.AddXunitTestLogging(TestOutput); }, hb => { hb.UseBootstrapperLifetime() .UseBootstrapperProgram(typeof(TestMinimalConsoleProgram)) .UseMinimalConsoleProgram(); }); + // Start the host explicitly for clarity + await test.Host.StartAsync(); await test.Host.WaitForShutdownAsync();
45-67: Consider moving commented code to documentation.The large block of commented code explaining deadlock scenarios is valuable information, but it clutters the test method. Consider moving it to a class-level comment or separate documentation.
If this information is important to preserve in the code, consider adding it as a well-formatted XML comment at the class level or creating a separate internal document that explains these concurrency considerations.
test/Codebelt.Bootstrapper.FunctionalTests/Assets/TestBackgroundService.cs (2)
14-14: Remove unused field_hostLifetimeThis field is declared but never used anywhere in the class. It should be removed to avoid confusion and maintain clean code.
- private readonly IHostLifetime _hostLifetime;
15-15: Addreadonlymodifier to_eventsfieldThe
_eventsfield should be marked asreadonlyfor consistency with other dependency fields and to ensure it cannot be reassigned after initialization.- private IHostLifetimeEvents _events; + private readonly IHostLifetimeEvents _events;app/Codebelt.Bootstrapper.MinimalWorker.App/FakeHostedService.cs (1)
12-13: Consider removing empty linesThere are two consecutive empty lines that could be consolidated to maintain consistent spacing throughout the file.
_logger = logger; - - events.OnApplicationStartedCallback = () => logger.LogInformation("Started");src/Codebelt.Bootstrapper.Console/ConsoleHostedService.cs (1)
15-15: Update class XML documentation to include IHostLifetimeEventsThe class XML documentation currently doesn't mention the dependency on
IHostLifetimeEvents. Consider updating it to reflect all major dependencies./// <summary> /// Provides a console application that is managed by its host. /// </summary> /// <typeparam name ="TStartup">The type containing the startup methods for the application.</typeparam> + /// <remarks> + /// This service depends on <see cref="IHostLifetimeEvents"/> to manage application lifecycle events. + /// </remarks> /// <seealso cref="IHostedService" />.github/workflows/pipelines.yml (1)
111-119: Improved CI workflow organization with external workflows.Restructuring the analysis jobs to use external workflows for SonarCloud, CodeCov, and CodeQL is a good practice for maintainability. This modular approach makes the main pipeline more concise and easier to manage.
Fix the spacing after commas in the needs array for better readability:
- needs: [build,test] + needs: [build, test]Also applies to: 121-127, 130-132
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 113-113: too few spaces after comma
(commas)
app/Codebelt.Bootstrapper.Worker.App/FakeHostedService.cs (2)
14-14: Consider null-checkingevents.It might be safer to guard against a null reference for
eventsto prevent potential NullReferenceExceptions, especially if dependency injection is misconfigured or an unexpected scenario occurs.
19-25: Use asynchronous delay to avoid blocking the thread.
Thread.Sleep(TimeSpan.FromSeconds(5))blocks the thread, which may impact performance or delay other tasks. Consider using an async approach (e.g.,await Task.Delay(...)) within an async callback if feasible, ensuring that it doesn't block the main thread unnecessarily.src/Codebelt.Bootstrapper/BootstrapperLifetime.cs (1)
25-34: Public Action delegates are reasonable but consider documentation updates.Exposing
OnApplicationStartedCallback,OnApplicationStoppingCallback, andOnApplicationStoppedCallbackasActiondelegates is clean. However, you may want to add or revise XML doc comments (particularly at line 33’s trailing text) to maintain clarity and fix the minor formatting overlap.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
.github/workflows/pipelines.yml(4 hunks)Codebelt.Bootstrapper.sln(3 hunks)Directory.Build.props(1 hunks)Directory.Packages.props(1 hunks)app/Codebelt.Bootstrapper.Console.App/Startup.cs(1 hunks)app/Codebelt.Bootstrapper.MinimalConsole.App/Program.cs(1 hunks)app/Codebelt.Bootstrapper.MinimalWorker.App/FakeHostedService.cs(1 hunks)app/Codebelt.Bootstrapper.Worker.App/FakeHostedService.cs(1 hunks)src/Codebelt.Bootstrapper.Console/ConsoleHostedService.cs(2 hunks)src/Codebelt.Bootstrapper.Console/HostApplicationBuilderExtensions.cs(2 hunks)src/Codebelt.Bootstrapper.Console/MinimalConsoleHostedService.cs(2 hunks)src/Codebelt.Bootstrapper.Console/MinimalConsoleProgram.cs(1 hunks)src/Codebelt.Bootstrapper.Web/MinimalWebProgram.cs(1 hunks)src/Codebelt.Bootstrapper.Worker/MinimalWorkerProgram.cs(1 hunks)src/Codebelt.Bootstrapper/BootstrapperLifetime.cs(2 hunks)src/Codebelt.Bootstrapper/HostApplicationBuilderExtensions.cs(1 hunks)src/Codebelt.Bootstrapper/HostBuilderExtensions.cs(1 hunks)src/Codebelt.Bootstrapper/HostedServiceExtensions.cs(0 hunks)src/Codebelt.Bootstrapper/IHostLifetimeEvents.cs(1 hunks)test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/ManualGenericHostFixture.cs(1 hunks)test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/ManualMinimalHostFixture.cs(1 hunks)test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestConsoleStartup.cs(1 hunks)test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestHostFixture.cs(1 hunks)test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestMinimalConsoleProgram.cs(1 hunks)test/Codebelt.Bootstrapper.Console.FunctionalTests/Codebelt.Bootstrapper.Console.FunctionalTests.csproj(1 hunks)test/Codebelt.Bootstrapper.Console.FunctionalTests/ConsoleHostedServiceTest.cs(1 hunks)test/Codebelt.Bootstrapper.Console.FunctionalTests/LoggerExtensions.cs(1 hunks)test/Codebelt.Bootstrapper.Console.FunctionalTests/MinimalConsoleHostedServiceTest.cs(1 hunks)test/Codebelt.Bootstrapper.FunctionalTests/Assets/TestBackgroundService.cs(1 hunks)test/Codebelt.Bootstrapper.FunctionalTests/Assets/TestHostFixture.cs(1 hunks)test/Codebelt.Bootstrapper.FunctionalTests/Assets/TestStartup.cs(1 hunks)test/Codebelt.Bootstrapper.FunctionalTests/BootstrapperLifetimeTest.cs(1 hunks)test/Codebelt.Bootstrapper.FunctionalTests/Codebelt.Bootstrapper.FunctionalTests.csproj(1 hunks)test/Codebelt.Bootstrapper.FunctionalTests/HostApplicationBuilderExtensionsTest.cs(1 hunks)test/Codebelt.Bootstrapper.FunctionalTests/HostBuilderExtensionsTest.cs(1 hunks)test/Codebelt.Bootstrapper.FunctionalTests/HostedServiceExtensionsTest.cs(1 hunks)test/Codebelt.Bootstrapper.Tests/Assets/StartupRootUnsafeAccessor.cs(1 hunks)test/Codebelt.Bootstrapper.Tests/Assets/TestStartupRoot.cs(1 hunks)test/Codebelt.Bootstrapper.Tests/Codebelt.Bootstrapper.Tests.csproj(1 hunks)test/Codebelt.Bootstrapper.Tests/StartupRootTest.cs(1 hunks)testenvironments.json(1 hunks)
💤 Files with no reviewable changes (1)
- src/Codebelt.Bootstrapper/HostedServiceExtensions.cs
🧰 Additional context used
🧬 Code Graph Analysis (15)
test/Codebelt.Bootstrapper.FunctionalTests/Assets/TestStartup.cs (1)
test/Codebelt.Bootstrapper.Tests/Assets/TestStartupRoot.cs (1)
ConfigureServices(14-18)
src/Codebelt.Bootstrapper/HostBuilderExtensions.cs (1)
src/Codebelt.Bootstrapper/BootstrapperLifetime.cs (2)
BootstrapperLifetime(17-96)BootstrapperLifetime(44-48)
test/Codebelt.Bootstrapper.Console.FunctionalTests/ConsoleHostedServiceTest.cs (4)
test/Codebelt.Bootstrapper.FunctionalTests/HostBuilderExtensionsTest.cs (2)
Fact(17-31)Fact(33-47)test/Codebelt.Bootstrapper.FunctionalTests/HostedServiceExtensionsTest.cs (1)
Fact(20-49)test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestConsoleStartup.cs (3)
Task(21-26)TestConsoleStartup(11-27)TestConsoleStartup(13-15)test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestMinimalConsoleProgram.cs (1)
Task(11-16)
test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestMinimalConsoleProgram.cs (2)
src/Codebelt.Bootstrapper.Console/MinimalConsoleProgram.cs (1)
MinimalConsoleProgram(11-33)test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestConsoleStartup.cs (1)
Task(21-26)
test/Codebelt.Bootstrapper.FunctionalTests/HostedServiceExtensionsTest.cs (2)
test/Codebelt.Bootstrapper.FunctionalTests/BootstrapperLifetimeTest.cs (2)
Fact(17-36)Fact(38-60)test/Codebelt.Bootstrapper.FunctionalTests/Assets/TestBackgroundService.cs (3)
Task(26-35)TestBackgroundService(10-36)TestBackgroundService(17-22)
test/Codebelt.Bootstrapper.FunctionalTests/HostApplicationBuilderExtensionsTest.cs (3)
test/Codebelt.Bootstrapper.FunctionalTests/BootstrapperLifetimeTest.cs (2)
Fact(17-36)Fact(38-60)test/Codebelt.Bootstrapper.FunctionalTests/HostBuilderExtensionsTest.cs (2)
Fact(17-31)Fact(33-47)test/Codebelt.Bootstrapper.FunctionalTests/HostedServiceExtensionsTest.cs (1)
Fact(20-49)
test/Codebelt.Bootstrapper.FunctionalTests/Assets/TestBackgroundService.cs (2)
test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestConsoleStartup.cs (1)
Task(21-26)test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestMinimalConsoleProgram.cs (1)
Task(11-16)
test/Codebelt.Bootstrapper.FunctionalTests/Assets/TestHostFixture.cs (1)
test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestHostFixture.cs (2)
TestHostFixture(9-34)ConfigureHost(11-33)
test/Codebelt.Bootstrapper.Tests/StartupRootTest.cs (2)
test/Codebelt.Bootstrapper.Tests/Assets/TestStartupRoot.cs (3)
TestStartupRoot(7-19)TestStartupRoot(9-12)ConfigureServices(14-18)test/Codebelt.Bootstrapper.Tests/Assets/StartupRootUnsafeAccessor.cs (1)
StartupRootUnsafeAccessor(7-14)
src/Codebelt.Bootstrapper.Console/MinimalConsoleHostedService.cs (2)
src/Codebelt.Bootstrapper.Console/ConsoleHostedService.cs (3)
Task(45-77)Task(79-83)Task(90-102)src/Codebelt.Bootstrapper.Console/MinimalConsoleProgram.cs (1)
MinimalConsoleProgram(11-33)
app/Codebelt.Bootstrapper.MinimalWorker.App/FakeHostedService.cs (1)
app/Codebelt.Bootstrapper.Worker.App/FakeHostedService.cs (2)
FakeHostedService(9-38)FakeHostedService(14-27)
app/Codebelt.Bootstrapper.Worker.App/FakeHostedService.cs (1)
app/Codebelt.Bootstrapper.MinimalWorker.App/FakeHostedService.cs (2)
FakeHostedService(3-33)FakeHostedService(8-22)
test/Codebelt.Bootstrapper.Console.FunctionalTests/MinimalConsoleHostedServiceTest.cs (2)
test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestConsoleStartup.cs (1)
Task(21-26)test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestMinimalConsoleProgram.cs (2)
Task(11-16)TestMinimalConsoleProgram(9-17)
test/Codebelt.Bootstrapper.Tests/Assets/TestStartupRoot.cs (1)
test/Codebelt.Bootstrapper.FunctionalTests/Assets/TestStartup.cs (1)
ConfigureServices(13-15)
test/Codebelt.Bootstrapper.FunctionalTests/BootstrapperLifetimeTest.cs (4)
test/Codebelt.Bootstrapper.FunctionalTests/HostApplicationBuilderExtensionsTest.cs (1)
Fact(23-34)test/Codebelt.Bootstrapper.FunctionalTests/HostBuilderExtensionsTest.cs (2)
Fact(17-31)Fact(33-47)test/Codebelt.Bootstrapper.FunctionalTests/HostedServiceExtensionsTest.cs (1)
Fact(20-49)test/Codebelt.Bootstrapper.FunctionalTests/Assets/TestStartup.cs (2)
TestStartup(7-16)TestStartup(9-11)
🪛 YAMLlint (1.35.1)
.github/workflows/pipelines.yml
[warning] 113-113: too few spaces after comma
(commas)
[warning] 123-123: too few spaces after comma
(commas)
[warning] 131-131: too few spaces after comma
(commas)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: 🧪 Test (ubuntu-24.04, Debug)
- GitHub Check: 📦 Pack (Debug)
- GitHub Check: 🧪 Test (ubuntu-24.04, Release)
- GitHub Check: 📦 Pack (Release)
🔇 Additional comments (52)
test/Codebelt.Bootstrapper.FunctionalTests/Codebelt.Bootstrapper.FunctionalTests.csproj (1)
1-11: Project structure looks clean and follows standard conventions.The project file is well-structured with appropriate namespace setup and project reference. The functional tests project is properly set up to reference the main Codebelt.Bootstrapper project.
src/Codebelt.Bootstrapper.Worker/MinimalWorkerProgram.cs (1)
17-19: Improved readability with step-by-step builder approach.Breaking down the host builder creation into discrete steps improves readability and maintainability. This aligns with the PR objective of enhancing abstraction and configuration in the project.
test/Codebelt.Bootstrapper.Console.FunctionalTests/Codebelt.Bootstrapper.Console.FunctionalTests.csproj (1)
1-11: Project structure is well-defined and follows conventions.The project file is properly set up with appropriate namespace and project reference to the Codebelt.Bootstrapper.Console project. The structure aligns with the new testing infrastructure described in the PR.
test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/ManualGenericHostFixture.cs (1)
5-11: Simple fixture for testing looks good.The
ManualGenericHostFixtureprovides a clean way to customize host behavior in tests by overriding theHostRunnerCallbackwith an empty implementation. This gives tests flexibility in controlling the host's lifecycle.Directory.Build.props (1)
76-76: Package reference update for test projects.The package reference has been updated from
Codebelt.Extensions.XunittoCodebelt.Extensions.Xunit.App, which aligns with the PR objectives to update package references. This change supports the improved abstraction goals of the PR.test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestConsoleStartup.cs (2)
1-28: Well-structured test class for console startup verification.This test class properly inherits from
ConsoleStartupand correctly passes configuration and environment to the base class. TheRunAsyncimplementation effectively demonstrates logging verification for testing purposes.
21-26: Good implementation of RunAsync for testing.The
RunAsyncmethod correctly obtains a logger through dependency injection and logs a trace message, making it easy to verify that the method was called during tests. The use ofnameofexpressions is a good practice for maintaining type safety.test/Codebelt.Bootstrapper.Tests/Assets/TestStartupRoot.cs (2)
1-20: Well-designed test class for startup functionality.This class properly extends
StartupRootand correctly passes configuration and environment to the base constructor. It provides a good example of how to overrideConfigureServicesfor testing purposes.
14-18: Effective service registration for testing.The
ConfigureServicesimplementation demonstrates how to register test services, which will be useful for verifying dependency injection behavior in tests. The comment helps clarify the purpose of this method.src/Codebelt.Bootstrapper.Web/MinimalWebProgram.cs (1)
12-12: Correct XML documentation update.The XML documentation now properly references
WebApplicationBuilderinstead ofHostApplicationBuilder, which matches the actual implementation and improves code documentation accuracy.app/Codebelt.Bootstrapper.MinimalConsole.App/Program.cs (2)
21-23: Good enhancement to lifecycle event managementThe code now retrieves and uses IHostLifetimeEvents interface instead of directly using BootstrapperLifetime. This aligns with dependency inversion principles by depending on abstractions rather than concrete implementations.
24-30: Well-implemented event callback handlersThe application lifecycle event callbacks are properly implemented using the IHostLifetimeEvents interface. The implementation keeps the same behavior (warning logs for starting/stopping and critical log for stopped) while improving the architecture.
test/Codebelt.Bootstrapper.FunctionalTests/HostBuilderExtensionsTest.cs (2)
17-31: Well-structured test for BootstrapperLifetime registrationThis test properly verifies that UseBootstrapperLifetime correctly registers the BootstrapperLifetime service. The test follows good practices:
- Clear, descriptive test name
- Proper use of the HostTestFactory
- Appropriate assertions to verify both existence and type of the service
33-47: Well-structured test for StartupFactory registrationThis test properly verifies that UseBootstrapperStartup correctly registers the StartupFactory service. The test maintains consistency with the previous test in structure and methodology.
test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestMinimalConsoleProgram.cs (1)
9-17: Good implementation of test helper classThe TestMinimalConsoleProgram class properly implements the abstract MinimalConsoleProgram class with a simple RunAsync method for testing purposes. The implementation follows the same pattern as TestConsoleStartup, maintaining consistency across test assets.
src/Codebelt.Bootstrapper.Console/MinimalConsoleProgram.cs (1)
20-24: Improved code readabilityThe refactoring of CreateHostBuilder from a chained method call to separate statements improves readability and maintainability without changing functionality. This approach makes debugging easier and the code flow clearer.
test/Codebelt.Bootstrapper.Console.FunctionalTests/ConsoleHostedServiceTest.cs (1)
1-43: Well-structured test class for validating console application lifecycle behaviorThis is a well-written test class that effectively validates the console application's lifecycle behavior. The test ensures that the RunAsync method in TestConsoleStartup is properly invoked during host execution.
The test:
- Creates a test host with Xunit logging
- Configures bootstrapper lifetime and the TestConsoleStartup
- Waits for the host to shutdown
- Verifies the expected log messages were generated
This provides good test coverage for the console startup flow.
src/Codebelt.Bootstrapper/HostApplicationBuilderExtensions.cs (4)
8-10: Updated documentation for interface referenceGood update to reference the interface instead of the concrete implementation in the documentation.
16-17: Updated documentation params and return for interfaceDocumentation appropriately updated to reference the interface.
19-20: Enhanced method signature with interface abstractionGood change from concrete
HostApplicationBuilderto theIHostApplicationBuilderinterface. This improves abstraction and flexibility.
21-22: Added registration for IHostLifetimeEventsExcellent addition that registers the IHostLifetimeEvents service, mapping it to the same instance as the BootstrapperLifetime. This enables dependency injection of the events interface without requiring direct casting in consumer code.
app/Codebelt.Bootstrapper.Console.App/Startup.cs (1)
26-42: Improved application lifecycle handling with IHostLifetimeEventsThe change from direct BootstrapperLifetime usage to the IHostLifetimeEvents interface is a good abstraction improvement. It decouples the startup class from the concrete lifetime implementation and follows the interface segregation principle.
The consistent use of lambda blocks for all callbacks also improves readability and maintainability.
test/Codebelt.Bootstrapper.FunctionalTests/BootstrapperLifetimeTest.cs (2)
1-16: Well-structured test setup with appropriate inheritance.The test class derives from a base Test class and properly initializes it with the ITestOutputHelper. This ensures consistent test infrastructure across the test suite.
17-36: Effective test for application startup callback.This test correctly verifies that the OnApplicationStartedCallback is invoked when the host starts. The test uses a clean pattern of arranging the services, setting up the callback, acting by starting the host, and asserting the expected outcome.
test/Codebelt.Bootstrapper.FunctionalTests/HostedServiceExtensionsTest.cs (1)
12-19: Well-structured test class setup.The test class correctly derives from the base Test class and properly initializes it with the ITestOutputHelper.
test/Codebelt.Bootstrapper.FunctionalTests/Assets/TestHostFixture.cs (1)
7-35: Well-implemented test host fixture.The TestHostFixture class correctly overrides the ConfigureHost method to set up a test host environment. The implementation follows best practices for configuring host builders and properly sets up the application configuration, services, and host configuration.
test/Codebelt.Bootstrapper.Console.FunctionalTests/MinimalConsoleHostedServiceTest.cs (1)
69-77: Good assertion pattern using log messages.The test correctly verifies behavior by asserting on log messages produced during execution. This is a robust way to test the execution flow of the application.
app/Codebelt.Bootstrapper.MinimalWorker.App/FakeHostedService.cs (1)
8-21: LGTM - Good refactoring to use IHostLifetimeEventsThe modification to use
IHostLifetimeEventsfollows the PR objective of refactoring application lifecycle event handling. This change is consistent with similar updates in other parts of the codebase.test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestHostFixture.cs (1)
11-33: LGTM - Well-implemented test fixtureThe
ConfigureHostmethod properly sets up the host builder with appropriate configurations for testing. The code follows good practices by using delegate callbacks for extensibility and utilizing theTestparameter to configure the application name.src/Codebelt.Bootstrapper.Console/ConsoleHostedService.cs (2)
23-23: LGTM - Good addition of IHostLifetimeEvents fieldAdding the
IHostLifetimeEventsfield aligns with the PR objective of refactoring lifecycle event handling across the codebase.
25-38: LGTM - Well-documented constructor parameterThe constructor and its documentation have been properly updated to include the new
IHostLifetimeEventsparameter.src/Codebelt.Bootstrapper.Console/HostApplicationBuilderExtensions.cs (1)
8-8: Great improvement to the interface abstraction!Replacing concrete
HostApplicationBuilderwithIHostApplicationBuilderinterface in method signatures and documentation improves the abstraction level of your code, making it more flexible and maintainable. This follows the Dependency Inversion Principle from SOLID and helps with future extensibility.Also applies to: 15-15, 17-18, 29-29
.github/workflows/pipelines.yml (3)
24-24: Good update to latest Ubuntu LTS version.Upgrading from ubuntu-22.04 to ubuntu-24.04 keeps your CI environment up-to-date with the latest LTS release.
Also applies to: 65-65
83-110: Well-structured test matrix for cross-platform validation.The new test job with matrix strategy for running tests on both Ubuntu and Windows is a great addition that ensures your code works consistently across platforms. The timeout setting and error handling configuration are also appropriate.
137-138: Updated deployment dependencies to include new jobs.The deployment job now correctly depends on all the required upstream jobs, ensuring that deployments only happen after successful testing and code analysis.
Codebelt.Bootstrapper.sln (1)
42-49: Good addition of test projects to the solution.Adding dedicated test projects improves the overall structure of the solution and enhances testability. The proper organization of projects into a dedicated "test" folder and configuration of build settings for both Debug and Release configurations is well done.
Also applies to: 120-131, 153-155
src/Codebelt.Bootstrapper.Console/MinimalConsoleHostedService.cs (2)
19-19: Good refactoring to support IHostLifetimeEvents.The updated field types and constructor parameter additions properly integrate with the new
IHostLifetimeEventsinterface. Using a genericILoggerinstead of the specificILogger<MinimalConsoleHostedService>allows for more flexible logging contexts.Also applies to: 22-22, 30-31, 36-36
46-76:Details
❓ Verification inconclusive
Improved application lifecycle management with event-based architecture.
Moving the startup logic into the
OnApplicationStartedCallbackaligns with the broader refactoring of application lifecycle events. The dynamic creation of a logger based on the program type is a nice touch that enhances log context.The enhanced error handling with specific log messages for different failure scenarios improves observability.
Consider potential thread safety issues when accessing shared fields from the callback, as it might be executed on a different thread than the
StartAsyncmethod:
🏁 Script executed:
#!/bin/bash # Searching for synchronization primitives or thread-safe collections in related classes # Look for usage of locks, semaphores, or other synchronization primitives rg -i "(lock|semaphore|concurrent|interlocked|volatile|threadsafe)" --type csharp # Check for accesses to shared state in lifecycle callbacks rg -i "OnApplication(Started|Stopping|Stopped)Callback" -A 10 -B 10 --type csharpLength of output: 34741
Review Update: Lifecycle Refactoring & Concurrency Caution
The refactored approach—shifting the startup logic into the
OnApplicationStartedCallbackand dynamically creating a logger based on the activated program type—indeed improves lifecycle management and enhances log context and error observability.One item to note:
- Thread Safety: While no explicit synchronization primitives (e.g., locks, volatile modifiers) were detected in the relevant areas, the callback assigns to shared fields (_logger, _runAsyncTask, _ranToCompletion) from a potentially different thread than the one executing
StartAsync. Please verify that these fields are safe for concurrent access or consider adding appropriate synchronization if there’s a chance of race conditions.app/Codebelt.Bootstrapper.Worker.App/FakeHostedService.cs (2)
18-18: Callback assignment looks good.Assigning
OnApplicationStartedCallbackto log a "Started" message is straightforward and aligns with typical lifecycle event usage.
26-26: Assignment toOnApplicationStoppedCallbackis fine.The logging statement upon application stop is appropriate and consistent with the existing lifecycle event assignments.
test/Codebelt.Bootstrapper.Tests/StartupRootTest.cs (5)
14-17: Fields for configuration and environment are clearly defined.Storing
_configurationand_environmentas private readonly fields ensures they are consistently accessible throughout the tests.
19-35: Constructor sets up test configuration and environment effectively.Using in-memory configuration and a hosting environment for development mode is a solid approach for unit tests. This setup is clear and ensures isolation from external config sources.
37-49: Test for configuration consistency is straightforward.Verifying that
StartupRootUnsafeAccessor.GetConfiguration(startup)returns the same configuration ensures correctness of the injected config property. Good coverage of the property’s behavior.
52-63: Environment property test logic is correct.Ensuring the environment instance matches the injected environment effectively validates the environment setup. This is very clear and maintainable.
65-79: Services test is well-structured.Testing that
TestStartupRoot.ConfigureServicesadds a known singleton service ("TestService") confirms the correctness of the service registration logic. This mirrors real-world usage scenarios properly.Directory.Packages.props (4)
7-10: Package version updates appear consistent.Upgrading to “Codebelt.Extensions.Xunit.App” (v10.0.0-preview.2), “Cuemon.Core” (v9.0.4), “Cuemon.Extensions.Hosting” (v9.0.4), and “Microsoft.NET.Test.Sdk” (v17.13.0) is a standard maintenance step. Verify compatibility and changelogs to avoid potential breaking changes.
15-15: xUnit runner version upgrade looks solid.Moving to
xunit.runner.visualstudio3.0.2 keeps the test runner up-to-date. Typically, this is a minor version improvement without high risk.
18-22: Minor version adjustments for .NET 9 packages.The updates to 9.0.4 for ASP.NET Core and Microsoft.Extensions libraries are incremental. Check release notes to ensure no breaking changes or newly introduced bugs.
25-26: .NET 8 package version bumps.Raising these versions to 8.0.15 is likely beneficial for bug fixes. As with other upgrades, ensure no regressions in your existing code paths.
src/Codebelt.Bootstrapper/BootstrapperLifetime.cs (3)
17-17: ImplementingIHostLifetimeEventsimproves design clarity.Switching to an instance-based approach for lifecycle callbacks increases flexibility compared to a static pattern. It aligns nicely with established .NET hosting patterns.
68-70: Registration of callbacks is well-handled.Binding
_applicationLifetimeevents to local handlers effectively orchestrates the lifecycle. This is a key part of switching from a console-based lifetime to a custom approach.
74-84: Local callback methods streamline the lifecycle.The private
OnApplicationStarted,OnApplicationStopped, andOnApplicationStoppingmethods keep logic contained and ensure each lifecycle event is handled independently. This is straightforward to maintain.
test/Codebelt.Bootstrapper.FunctionalTests/HostApplicationBuilderExtensionsTest.cs
Show resolved
Hide resolved
test/Codebelt.Bootstrapper.FunctionalTests/Assets/TestBackgroundService.cs
Show resolved
Hide resolved
test/Codebelt.Bootstrapper.Console.FunctionalTests/Assets/TestHostFixture.cs
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9 +/- ##
=======================================
Coverage ? 85.43%
=======================================
Files ? 13
Lines ? 206
Branches ? 15
=======================================
Hits ? 176
Misses ? 30
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This pull request includes updates to the
.github/workflows/pipelines.ymlfile, modifications to theCodebelt.Bootstrapper.slnsolution file, updates to package references, and changes to how application lifecycle events are handled in various services.Workflow and Build Configuration Updates:
ubuntu-24.04and added a matrix configuration for thetestjob to run on bothubuntu-24.04andwindows-2022. [1] [2] [3]sonarcloudjob with a newtestjob and moved thesonarcloudanalysis to be called from a separate workflow file. [1] [2]codecovjob to call Codecov analysis from a separate workflow file.Solution and Project File Updates:
Codebelt.Bootstrapper.slnsolution file and updated solution configurations to include these new projects. [1] [2] [3]Directory.Build.propsandDirectory.Packages.propsto newer versions. [1] [2]Application Lifecycle Event Handling:
BootstrapperLifetimecallbacks withIHostLifetimeEventsin various services and applications. [1] [2] [3] [4]ConsoleHostedServiceto useIHostLifetimeEventsfor managing lifecycle events. [1] [2]Codebase Improvements:
HostApplicationBuilderExtensionsto useIHostApplicationBuilderinstead ofHostApplicationBuilderfor better interface abstraction.Summary by CodeRabbit