Skip to content

Conversation

@gimlichael
Copy link
Member

@gimlichael gimlichael commented Oct 6, 2024

PR Classification

Code enhancement and new feature implementation.

PR Summary

Enhanced service handling and added support for scoped services with improved test coverage.

  • AspNetCoreHostFixture.cs: Added conditional compilation for .NET 9.0+ and ensured asynchronous host start,
  • FakeHttpContextAccessor: Introduced IServiceScopeFactory and updated initialization logic,
  • ServiceCollectionExtensions.cs: Modified AddFakeHttpContextAccessor to default to Singleton lifetime,
  • HostFixture.cs: Added thread-safety to Dispose method,
  • AspNetCoreHostTestTest.cs: Added new tests for scoped services and correlation tokens.

Summary by CodeRabbit

  • New Features

    • Enhanced configurability and functionality of the FakeHttpContextAccessor with service provider integration.
    • Introduced new correlation token types: ScopedCorrelation, SingletonCorrelation, and TransientCorrelation.
    • Simplified service registration process by allowing default service lifetime in AddFakeHttpContextAccessor.
  • Bug Fixes

    • Improved thread safety and validation in the HostFixture class during host configuration and disposal.
  • Tests

    • Added new tests for correlation token behaviors and improved service configuration validation.
  • Chores

    • Updated package references to the latest preview versions.

@gimlichael gimlichael self-assigned this Oct 6, 2024
@coderabbitai
Copy link

coderabbitai bot commented Oct 6, 2024

Caution

Review failed

The pull request is closed.

Walkthrough

The pull request introduces several modifications across multiple classes in the Codebelt.Extensions.Xunit.Hosting.AspNetCore namespace. Key changes include the addition of conditional compilation for .NET 9.0 in the AspNetCoreHostFixture, updates to the FakeHttpContextAccessor constructor for improved dependency injection, and enhancements to service registration methods. New test methods are added to validate correlation token behaviors, and several new record types for correlation tokens are introduced. Additionally, updates to project files reflect version increments for dependencies without structural changes.

Changes

File Change Summary
src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/AspNetCoreHostFixture.cs Added conditional compilation for .NET 9.0 in ConfigureHost to validate service provider settings.
src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/Http/FakeHttpContextAccessor.cs Modified FakeHttpContextAccessor constructor to accept IServiceScopeFactory and enhanced HttpContext initialization with FeatureCollection.
src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/ServiceCollectionExtensions.cs Updated AddFakeHttpContextAccessor method to have a default ServiceLifetime.Singleton parameter and modified the instantiation of FakeHttpContextAccessor to use IServiceProvider.
src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/WebHostTest.cs Simplified service registration in ConfigureServices by removing explicit lifetime specification for AddFakeHttpContextAccessor.
src/Codebelt.Extensions.Xunit.Hosting/HostFixture.cs Added thread-safe disposal logic and updated ConfigureHost for service provider validation. Introduced asynchronous host start logic.
test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/AspNetCoreHostTestTest.cs Added new tests for correlation token behaviors and updated service configuration in ConfigureServices.
test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/Assets/ScopedCorrelation.cs Introduced new record ScopedCorrelation for correlation token management.
test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/Assets/SingletonCorrelation.cs Introduced new record SingletonCorrelation for correlation token management.
test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/Assets/TransientCorrelation.cs Introduced new record TransientCorrelation for correlation token management.
test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests.csproj Updated package references for Cuemon.Extensions.AspNetCore and Cuemon.Extensions.IO to version 9.0.0-preview.10.
test/Codebelt.Extensions.Xunit.Hosting.Tests/Codebelt.Extensions.Xunit.Hosting.Tests.csproj Updated Cuemon.Core package reference to version 9.0.0-preview.10 and included appsettings.json in the project.
test/Codebelt.Extensions.Xunit.Hosting.Tests/HostTestTest.cs Added _scope field for service scope management and updated service retrieval logic in tests. Removed obsolete test method and added resource disposal logic.

Possibly related PRs

🐇 In the meadow, changes bloom,
New tokens sprout, dispelling gloom.
With each test, we hop and play,
Validations guide our way.
In the code, we find our cheer,
For every change, we hold dear! 🌼


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🧹 Outside diff range and nitpick comments (7)
test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/Assets/ScopedCorrelation.cs (1)

5-7: Consider adding XML documentation.

To improve code clarity and maintainability, consider adding XML documentation to the ScopedCorrelation record. This would help other developers understand its purpose and usage within the test suite.

Here's a suggested implementation:

+    /// <summary>
+    /// Represents a scoped correlation token for testing purposes.
+    /// </summary>
     public sealed record ScopedCorrelation : CorrelationToken
     {
     }
test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/Assets/SingletonCorrelation.cs (1)

1-8: LGTM! Consider adding documentation for clarity.

The implementation of SingletonCorrelation as a sealed record inheriting from CorrelationToken is a good choice for an immutable correlation token type. This aligns well with the PR objectives of improving service handling and HTTP context.

Consider adding XML documentation to clarify the intended usage of this correlation token, especially its singleton nature. For example:

/// <summary>
/// Represents a correlation token intended for singleton usage within the application.
/// </summary>
/// <remarks>
/// This token type is useful for scenarios where a single, application-wide correlation identifier is needed.
/// </remarks>
public sealed record SingletonCorrelation : CorrelationToken
{
}

This documentation would help other developers understand the purpose and proper usage of this token type.

test/Codebelt.Extensions.Xunit.Hosting.Tests/Codebelt.Extensions.Xunit.Hosting.Tests.csproj (1)

Line range hint 8-10: LGTM. Consider removing the redundant None Remove item.

The inclusion of appsettings.json as content with CopyToOutputDirectory set to Always is appropriate for configuration files in test projects. This ensures that the configuration file is available during test execution.

However, the None Remove item for appsettings.json (lines 8-10) appears to be redundant with the new Content Include. Consider removing it to clean up the project file:

-  <ItemGroup>
-    <None Remove="appsettings.json" />
-  </ItemGroup>

Also applies to: 12-14

src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/ServiceCollectionExtensions.cs (1)

42-42: Improved dependency injection in FakeHttpContextAccessorFactory.

The update to use dependency injection for retrieving IServiceScopeFactory is a good improvement. It allows for better control over service lifetimes within FakeHttpContextAccessor.

However, consider wrapping the GetRequiredService call in a try-catch block to provide a more informative error message if IServiceScopeFactory is not registered.

Here's a suggested improvement:

- return new FakeHttpContextAccessor(provider.GetRequiredService<IServiceScopeFactory>());
+ try
+ {
+     return new FakeHttpContextAccessor(provider.GetRequiredService<IServiceScopeFactory>());
+ }
+ catch (InvalidOperationException ex)
+ {
+     throw new InvalidOperationException("Failed to create FakeHttpContextAccessor. Ensure IServiceScopeFactory is registered in the service collection.", ex);
+ }
src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/AspNetCoreHostFixture.cs (1)

Line range hint 1-124: Overall assessment: Positive enhancement with minor considerations

The addition of the conditional compilation block for .NET 9.0+ enhances the AspNetCoreHostFixture class by providing stricter service validation in newer .NET versions. This change maintains backward compatibility while leveraging new features, which is a good practice in library development.

To further improve the class:

  1. Consider extracting the host configuration logic into separate methods to improve readability and maintainability of the ConfigureHost method.
  2. Evaluate if any of the new .NET 9.0+ features could be backported or if there are equivalent implementations for earlier versions to maintain consistent behavior across .NET versions where possible.
src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/Http/FakeHttpContextAccessor.cs (1)

24-27: Consider making the greeting message configurable or more generic.

The hardcoded greeting in MakeGreeting("Hello awesome developers!") might not be suitable for all testing scenarios. It could be beneficial to make the greeting message configurable or use a more generic placeholder to prevent any unintended effects in tests.

Apply this diff to make the greeting configurable:

- fc.Set<IHttpResponseBodyFeature>(new StreamResponseBodyFeature(MakeGreeting("Hello awesome developers!")));
+ fc.Set<IHttpResponseBodyFeature>(new StreamResponseBodyFeature(MakeGreeting(greeting: "Test Response")));

Or consider adding a parameter to the constructor to pass a custom greeting.

test/Codebelt.Extensions.Xunit.Hosting.Tests/HostTestTest.cs (1)

Line range hint 28-35: Shared state detected due to static ScopedCorrelations collection

Adding ScopedCorrelation instances to a static collection can cause tests to interact with each other's data, leading to unreliable test results.

After making ScopedCorrelations an instance field, ensure that each test operates on its own instance to maintain test isolation.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 4576167 and 69a0a0e.

📒 Files selected for processing (12)
  • src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/AspNetCoreHostFixture.cs (1 hunks)
  • src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/Http/FakeHttpContextAccessor.cs (2 hunks)
  • src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/ServiceCollectionExtensions.cs (2 hunks)
  • src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/WebHostTest.cs (1 hunks)
  • src/Codebelt.Extensions.Xunit.Hosting/HostFixture.cs (4 hunks)
  • test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/AspNetCoreHostTestTest.cs (5 hunks)
  • test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/Assets/ScopedCorrelation.cs (1 hunks)
  • test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/Assets/SingletonCorrelation.cs (1 hunks)
  • test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/Assets/TransientCorrelation.cs (1 hunks)
  • test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests.csproj (1 hunks)
  • test/Codebelt.Extensions.Xunit.Hosting.Tests/Codebelt.Extensions.Xunit.Hosting.Tests.csproj (1 hunks)
  • test/Codebelt.Extensions.Xunit.Hosting.Tests/HostTestTest.cs (4 hunks)
✅ Files skipped from review due to trivial changes (1)
  • test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests.csproj
🧰 Additional context used
🔇 Additional comments (19)
test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/Assets/ScopedCorrelation.cs (1)

1-8: Implementation looks good.

The ScopedCorrelation record is correctly defined as a sealed record inheriting from CorrelationToken. This structure is suitable for its likely use in testing scoped correlation scenarios.

test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/Assets/TransientCorrelation.cs (3)

1-1: LGTM: Appropriate namespace import.

The import of Cuemon.Messaging is correct, as it likely contains the CorrelationToken class that TransientCorrelation inherits from.


3-4: LGTM: Appropriate namespace declaration.

The namespace Codebelt.Extensions.Xunit.Hosting.AspNetCore.Assets correctly reflects the file's location in the test assets directory and its purpose.


5-7: LGTM, but clarification needed on the record's purpose.

The TransientCorrelation record is correctly declared as sealed and inherits from CorrelationToken. However, it's currently empty and doesn't add any new properties or behavior.

Could you please clarify the purpose of this separate type? If it's intended to be used as a marker type for transient correlations, consider adding a comment explaining its usage.

To better understand the usage of this new type, let's search for its occurrences in the codebase:

✅ Verification successful

Approved. The TransientCorrelation record is appropriately used in tests to differentiate transient correlation tokens. Although it doesn't add new properties or behavior now, it allows for clear and flexible testing of different lifetimes.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usages of TransientCorrelation
rg --type csharp -A 5 "TransientCorrelation"

Length of output: 2706

test/Codebelt.Extensions.Xunit.Hosting.Tests/Codebelt.Extensions.Xunit.Hosting.Tests.csproj (1)

18-18: LGTM. Verify compatibility with the updated package.

The update of the Cuemon.Core package to version 9.0.0-preview.10 is a good practice to keep dependencies current. However, as this is a preview version, it's important to ensure that this update doesn't introduce any breaking changes or unexpected behavior in the tests.

Please run the following script to check for any potential issues:

src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/ServiceCollectionExtensions.cs (2)

19-21: Improved method signature with sensible default.

The addition of a default value (ServiceLifetime.Singleton) for the lifetime parameter is a good improvement. It enhances usability by allowing callers to omit the parameter when the default behavior is desired, while maintaining backwards compatibility. The choice of Singleton as the default lifetime is appropriate for IHttpContextAccessor, as it's typically used as a global service.


Line range hint 1-45: Overall structure and consistency maintained.

The changes in this file are well-integrated and maintain consistency with the existing code style and structure. The modifications align with the PR objectives of improving host consistency and HTTP context handling. No issues with the overall file structure or consistency are apparent.

src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/WebHostTest.cs (1)

86-86: LGTM! Verify default lifetime in ServiceCollectionExtensions.

The simplification of AddFakeHttpContextAccessor() call aligns with the PR objectives of improving service handling. This change suggests that the default lifetime for AddFakeHttpContextAccessor has been set to Singleton in ServiceCollectionExtensions.cs.

To ensure consistency, please verify the default lifetime setting in ServiceCollectionExtensions.cs:

✅ Verification successful

Verified: Default lifetime is Singleton. No issues found.

This confirms that AddFakeHttpContextAccessor() now defaults to Singleton as intended.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the default lifetime for AddFakeHttpContextAccessor in ServiceCollectionExtensions.cs

# Test: Search for the AddFakeHttpContextAccessor method definition
# Expect: The method should have a default parameter value of ServiceLifetime.Singleton
rg --type csharp -A 10 'public static IServiceCollection AddFakeHttpContextAccessor' src

Length of output: 1538

src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/AspNetCoreHostFixture.cs (1)

83-89: Approve with suggestions: Enhanced service provider configuration for .NET 9.0+

The addition of UseDefaultServiceProvider for .NET 9.0 and above is a good improvement, enhancing service validation in the test environment. This change aligns with best practices for dependency injection in ASP.NET Core.

Consider adding a comment explaining the rationale behind this change and its potential impacts. For example:

#if NET9_0_OR_GREATER
// Configure stricter validation for .NET 9.0+ to catch DI issues early in the test environment
hb.UseDefaultServiceProvider(o =>
{
    o.ValidateOnBuild = true;
    o.ValidateScopes = true;
});
#endif

Please verify the following aspects:

  1. Cross-version testing: Ensure that tests run consistently across different .NET versions, given this conditional behavior.
  2. Performance impact: Validate that the additional validation doesn't significantly impact test execution time.

You can use the following script to check for any existing performance tests or benchmarks:

This will help identify if there are existing performance tests that should be updated or if new ones should be added to monitor any potential impact.

src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/Http/FakeHttpContextAccessor.cs (4)

7-7: Add missing using directive for IServiceScopeFactory.

The addition of using Microsoft.Extensions.DependencyInjection; is necessary for the IServiceScopeFactory interface used in the constructor.


20-20: Confirm the necessity of the optional IServiceScopeFactory parameter.

While adding IServiceScopeFactory enhances dependency injection capabilities, ensure that all usages of FakeHttpContextAccessor accommodate this change and that the default value of null does not introduce any unintended side effects.


28-29: Ensure proper reinitialization of HttpContext.

Calling context.Uninitialize(); followed by context.Initialize(fc); is correct for reinitializing the HttpContext with the new FeatureCollection. Confirm that this sequence is necessary and does not have any side effects.


24-24: ⚠️ Potential issue

Handle potential null reference when factory is null.

Since factory can be null, ensure that passing it to RequestServicesFeature does not result in a NullReferenceException. Verify that RequestServicesFeature can handle a null IServiceScopeFactory without issues.

test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/AspNetCoreHostTestTest.cs (3)

38-39: Logging Statements Added to Test Method

The addition of logging within the ShouldHaveResultOfBoolMiddlewareInBody method enhances test output and aids in debugging. The logger is correctly initialized and used.


65-88: Tests Correctly Verify Service Lifetimes

The test ShouldHaveAccessToCorrelationTokens_UsingScopedProvider accurately verifies the behavior of singleton, transient, and scoped services within a scope. The assertions ensure that:

  • Singleton instances are the same across requests.
  • Transient instances are different across requests.
  • Scoped instances are the same within the same scope.

146-148: Service Registrations for ICorrelationToken

Registering ICorrelationToken with singleton, transient, and scoped lifetimes is appropriate for testing different service behaviors. This setup allows for thorough testing of service lifetimes and is correctly utilized in the tests.

src/Codebelt.Extensions.Xunit.Hosting/HostFixture.cs (3)

16-17: Adding a synchronization lock object for thread safety

The addition of the private readonly object _lock = new(); field provides a synchronization mechanism to ensure thread safety in the Dispose method. This helps prevent race conditions when disposing resources from multiple threads.


61-65: Enabling service provider validation to catch configuration errors early

Setting ValidateOnBuild and ValidateScopes to true enhances the application's robustness by ensuring that any issues with service registrations are caught during the host build process. This proactive validation helps detect misconfigured or missing services early in the development cycle.


200-208: Ensuring thread-safe disposal in the Dispose method

Wrapping the disposal logic within a lock statement ensures that the Dispose method is thread-safe. This prevents multiple threads from disposing of the object simultaneously, which could lead to inconsistent states or exceptions.

/// Initializes a new instance of the <see cref="FakeHttpContextAccessor"/> class.
/// </summary>
public FakeHttpContextAccessor()
public FakeHttpContextAccessor(IServiceScopeFactory factory = null)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update XML documentation to reflect constructor changes.

The constructor now accepts an optional IServiceScopeFactory, but the XML documentation has not been updated to describe this parameter. Please update the comments to maintain accurate documentation.

Apply this diff to update the XML documentation:

 /// <summary>
-/// Initializes a new instance of the <see cref="FakeHttpContextAccessor"/> class.
+/// Initializes a new instance of the <see cref="FakeHttpContextAccessor"/> class with an optional <see cref="IServiceScopeFactory"/>.
 /// </summary>
+/// <param name="factory">An optional service scope factory for resolving services.</param>
 public FakeHttpContextAccessor(IServiceScopeFactory factory = null)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public FakeHttpContextAccessor(IServiceScopeFactory factory = null)
/// <summary>
/// Initializes a new instance of the <see cref="FakeHttpContextAccessor"/> class with an optional <see cref="IServiceScopeFactory"/>.
/// </summary>
/// <param name="factory">An optional service scope factory for resolving services.</param>
public FakeHttpContextAccessor(IServiceScopeFactory factory = null)

Comment on lines +23 to +24
_scope = hostFixture.ServiceProvider.CreateScope();
_correlationsFactory = () => _scope.ServiceProvider.GetServices<ICorrelationToken>().ToList();
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Reusing the same IServiceScope across tests may cause unintended shared state

Creating a single IServiceScope in the constructor and reusing it across multiple tests can lead to shared instances of scoped services, which might result in tests affecting each other.

Consider creating a new IServiceScope within each test method to ensure proper isolation. This can be achieved by initializing the scope at the beginning of each test and disposing of it at the end.

Comment on lines +17 to +19
private readonly IServiceScope _scope;
private readonly Func<IList<ICorrelationToken>> _correlationsFactory;
private static readonly ConcurrentBag<ICorrelationToken> ScopedCorrelations = new ConcurrentBag<ICorrelationToken>();
private static readonly ConcurrentBag<ICorrelationToken> ScopedCorrelations = new();
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Avoid using static fields in test classes to prevent shared state across tests

The field ScopedCorrelations is declared as static, which can lead to shared state between tests and potentially cause flaky tests due to unintended interactions.

Consider making ScopedCorrelations an instance field:

-private static readonly ConcurrentBag<ICorrelationToken> ScopedCorrelations = new();
+private readonly ConcurrentBag<ICorrelationToken> ScopedCorrelations = new();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private readonly IServiceScope _scope;
private readonly Func<IList<ICorrelationToken>> _correlationsFactory;
private static readonly ConcurrentBag<ICorrelationToken> ScopedCorrelations = new ConcurrentBag<ICorrelationToken>();
private static readonly ConcurrentBag<ICorrelationToken> ScopedCorrelations = new();
private readonly IServiceScope _scope;
private readonly Func<IList<ICorrelationToken>> _correlationsFactory;
private readonly ConcurrentBag<ICorrelationToken> ScopedCorrelations = new();

Comment on lines +66 to +69
protected override void OnDisposeManagedResources()
{
_scope?.Dispose();
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Override OnDisposeManagedResources should call base class method

When overriding OnDisposeManagedResources, it's important to call the base class implementation to ensure that any disposal logic in the base class is executed.

Apply this diff to call the base class method:

protected override void OnDisposeManagedResources()
{
+    base.OnDisposeManagedResources();
    _scope?.Dispose();
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
protected override void OnDisposeManagedResources()
{
_scope?.Dispose();
}
protected override void OnDisposeManagedResources()
{
base.OnDisposeManagedResources();
_scope?.Dispose();
}

[Fact]
public void ShouldHaveAccessToCorrelationTokens_UsingRequestServices() // reference: https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http/src/Features/RequestServicesFeature.cs
{
var context = _provider.GetRequiredService<IHttpContextAccessor>().HttpContext!;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid Using the Null-Forgiving Operator

Using the null-forgiving operator ! can mask potential NullReferenceException issues. Instead, explicitly check for null to ensure the test fails with a meaningful message if HttpContext is not available.

Consider modifying the code:

-var context = _provider.GetRequiredService<IHttpContextAccessor>().HttpContext!;
+var httpContextAccessor = _provider.GetRequiredService<IHttpContextAccessor>();
+Assert.NotNull(httpContextAccessor.HttpContext);
+var context = httpContextAccessor.HttpContext!;

This way, the test will explicitly assert that HttpContext is not null before proceeding.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var context = _provider.GetRequiredService<IHttpContextAccessor>().HttpContext!;
var httpContextAccessor = _provider.GetRequiredService<IHttpContextAccessor>();
Assert.NotNull(httpContextAccessor.HttpContext);
var context = httpContextAccessor.HttpContext!;

Comment on lines +53 to +63
#if NET9_0_OR_GREATER
[Fact]
public void ShouldThrowInvalidOperationException_BecauseOneOfTheServicesIsScoped()
{
var ex = Assert.Throws<InvalidOperationException>(() => _provider.GetServices<ICorrelationToken>());

TestOutput.WriteLine(ex.Message);

Assert.Contains("from root provider because it requires scoped service", ex.Message);
}
#endif
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve Exception Message Assertion for Robustness

When asserting on exception messages, consider that they may vary between .NET versions or due to localization. To make the test more robust, use culture-invariant checks or assert on exception properties if available.

Consider modifying the assertion as follows:

-Assert.Contains("from root provider because it requires scoped service", ex.Message);
+Assert.Contains("requires scoped service", ex.Message, StringComparison.OrdinalIgnoreCase);

Alternatively, if the exception provides specific properties or error codes, assert on those to avoid dependency on message text.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#if NET9_0_OR_GREATER
[Fact]
public void ShouldThrowInvalidOperationException_BecauseOneOfTheServicesIsScoped()
{
var ex = Assert.Throws<InvalidOperationException>(() => _provider.GetServices<ICorrelationToken>());
TestOutput.WriteLine(ex.Message);
Assert.Contains("from root provider because it requires scoped service", ex.Message);
}
#endif
#if NET9_0_OR_GREATER
[Fact]
public void ShouldThrowInvalidOperationException_BecauseOneOfTheServicesIsScoped()
{
var ex = Assert.Throws<InvalidOperationException>(() => _provider.GetServices<ICorrelationToken>());
TestOutput.WriteLine(ex.Message);
Assert.Contains("requires scoped service", ex.Message, StringComparison.OrdinalIgnoreCase);
}
#endif

Comment on lines +70 to +75
var host = hb.Build();
Task.Run(() => host.StartAsync().ConfigureAwait(false))
.ConfigureAwait(false)
.GetAwaiter()
.GetResult();
Host = host;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Simplify host startup to avoid unnecessary Task.Run and potential deadlocks

The current implementation wraps host.StartAsync() inside a Task.Run and then synchronously waits for it using .GetAwaiter().GetResult(). This pattern can introduce unnecessary complexity and potential deadlocks. Since StartAsync() returns a Task, you can synchronously wait on it directly.

Apply this diff to simplify the host startup:

 var host = hb.Build();
-Task.Run(() => host.StartAsync().ConfigureAwait(false))
-    .ConfigureAwait(false)
-    .GetAwaiter()
-    .GetResult();
+host.StartAsync().GetAwaiter().GetResult();
 Host = host;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var host = hb.Build();
Task.Run(() => host.StartAsync().ConfigureAwait(false))
.ConfigureAwait(false)
.GetAwaiter()
.GetResult();
Host = host;
var host = hb.Build();
host.StartAsync().GetAwaiter().GetResult();
Host = host;

@codecov
Copy link

codecov bot commented Oct 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.62%. Comparing base (4576167) to head (69a0a0e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #15      +/-   ##
==========================================
- Coverage   75.20%   68.62%   -6.58%     
==========================================
  Files          31       20      -11     
  Lines         621      408     -213     
  Branches       79       59      -20     
==========================================
- Hits          467      280     -187     
+ Misses        150      126      -24     
+ Partials        4        2       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 6, 2024

@gimlichael gimlichael merged commit 2010a78 into main Oct 6, 2024
@gimlichael gimlichael deleted the v9.0.0/host-consistency-and-httpcontext-improvement branch October 6, 2024 12:20
@coderabbitai coderabbitai bot mentioned this pull request Nov 11, 2024
@coderabbitai coderabbitai bot mentioned this pull request Nov 11, 2025
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