V9.0.0/prepare for launch #20
Conversation
WalkthroughThis pull request introduces significant updates for version 9.0.0, enhancing compatibility with .NET 9 preview releases. Key additions include new methods in various classes and interfaces, particularly for asynchronous disposal. Package references have been streamlined by removing specific version numbers, allowing for more flexible dependency management. The project structure has been adjusted to facilitate these changes, including the addition of a centralized package management file. Overall, the modifications aim to ensure production readiness and improve resource management. Changes
Possibly related PRs
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? 🪧 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
=======================================
Coverage 85.58% 85.58%
=======================================
Files 31 31
Lines 680 680
Branches 84 84
=======================================
Hits 582 582
Misses 95 95
Partials 3 3 ☔ View full report in Codecov by Sentry. |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
test/Codebelt.Extensions.Xunit.Tests/Assets/UnmanagedDisposable.cs (3)
Line range hint
31-89: Add error handling and platform-specific file pathsThe current implementation has several areas that need attention:
- The Windows-specific file path (
C:\TestFile.txt) won't work on Linux systems- Missing error handling for failed native library operations
- Potential resource leak if CreateFileW succeeds but subsequent operations fail
Consider applying these improvements:
#if NET8_0_OR_GREATER if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { + string testFilePath = Path.Combine(Path.GetTempPath(), "TestFile.txt"); if (NativeLibrary.TryLoad("kernel32.dll", GetType().Assembly, DllImportSearchPath.System32, out _libHandle)) { if (NativeLibrary.TryGetExport(_libHandle, "CreateFileW", out var functionHandle)) { var createFileFunc = Marshal.GetDelegateForFunctionPointer<CreateFileDelegate>(functionHandle); - _handle = createFileFunc(@"C:\TestFile.txt", + _handle = createFileFunc(testFilePath, 0x80000000, //access read-only 1, //share-read IntPtr.Zero, 3, //open existing 0, IntPtr.Zero); + if (_handle == new IntPtr(-1)) + { + throw new InvalidOperationException($"Failed to create test file. Error: {Marshal.GetLastWin32Error()}"); + } } + else + { + throw new InvalidOperationException("Failed to get CreateFileW export"); + } } + else + { + throw new InvalidOperationException("Failed to load kernel32.dll"); + } }
Line range hint
92-124: Improve resource cleanup reliabilityThe current disposal implementation could be more robust:
- Missing null checks for
_libHandle- No error handling for failed
CloseHandleoperations- Potential handle leaks if
GetExportfailsConsider applying these improvements:
#if NET8_0_OR_GREATER if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { - if (_handle != IntPtr.Zero) + if (_handle != IntPtr.Zero && _handle != new IntPtr(-1)) { - if (NativeLibrary.TryGetExport(_libHandle, "CloseHandle", out var closeHandle)) + if (_libHandle != IntPtr.Zero && NativeLibrary.TryGetExport(_libHandle, "CloseHandle", out var closeHandle)) { var closeHandleAction = Marshal.GetDelegateForFunctionPointer<CloseHandle>(closeHandle); - closeHandleAction(_handle); + if (!closeHandleAction(_handle)) + { + // Log or handle the error, but continue cleanup + Debug.WriteLine($"Failed to close handle. Error: {Marshal.GetLastWin32Error()}"); + } + _handle = IntPtr.Zero; } } - NativeLibrary.Free(_libHandle); + if (_libHandle != IntPtr.Zero) + { + NativeLibrary.Free(_libHandle); + _libHandle = IntPtr.Zero; + } }
Line range hint
1-124: Consider architectural improvementsA few architectural considerations:
The class name and inheritance from
Testsuggest this is a test class, but it appears to be a test helper or fixture. Consider renaming toUnmanagedDisposableFixtureor similar to better reflect its purpose.The dual implementation approach (.NET 8+ vs older) increases maintenance burden. Consider extracting the platform-specific logic into separate strategy classes to improve maintainability.
Would you like assistance in designing a more maintainable architecture for this test utility?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
CHANGELOG.md(1 hunks)Directory.Build.props(2 hunks)Directory.Packages.props(1 hunks)README.md(1 hunks)src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/Codebelt.Extensions.Xunit.Hosting.AspNetCore.csproj(1 hunks)src/Codebelt.Extensions.Xunit.Hosting/Codebelt.Extensions.Xunit.Hosting.csproj(1 hunks)src/Codebelt.Extensions.Xunit/Codebelt.Extensions.Xunit.csproj(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.Tests/Assets/UnmanagedDisposable.cs(2 hunks)test/Codebelt.Extensions.Xunit.Tests/Codebelt.Extensions.Xunit.Tests.csproj(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests/Codebelt.Extensions.Xunit.Hosting.AspNetCore.Tests.csproj
- test/Codebelt.Extensions.Xunit.Tests/Codebelt.Extensions.Xunit.Tests.csproj
🧰 Additional context used
📓 Learnings (3)
src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/Codebelt.Extensions.Xunit.Hosting.AspNetCore.csproj (1)
Learnt from: gimlichael
PR: codebeltnet/xunit#17
File: src/Codebelt.Extensions.Xunit.Hosting/Codebelt.Extensions.Xunit.Hosting.csproj:22-26
Timestamp: 2024-10-09T14:10:40.721Z
Learning: In the `src/Codebelt.Extensions.Xunit.Hosting/Codebelt.Extensions.Xunit.Hosting.csproj` file, only certain .NET 8 packages are available in version 8.0.1, while others remain at 8.0.0, so package versions may differ due to availability.
src/Codebelt.Extensions.Xunit.Hosting/Codebelt.Extensions.Xunit.Hosting.csproj (1)
Learnt from: gimlichael
PR: codebeltnet/xunit#17
File: src/Codebelt.Extensions.Xunit.Hosting/Codebelt.Extensions.Xunit.Hosting.csproj:22-26
Timestamp: 2024-10-09T14:10:40.721Z
Learning: In the `src/Codebelt.Extensions.Xunit.Hosting/Codebelt.Extensions.Xunit.Hosting.csproj` file, only certain .NET 8 packages are available in version 8.0.1, while others remain at 8.0.0, so package versions may differ due to availability.
test/Codebelt.Extensions.Xunit.Hosting.Tests/Codebelt.Extensions.Xunit.Hosting.Tests.csproj (1)
Learnt from: gimlichael
PR: codebeltnet/xunit#17
File: src/Codebelt.Extensions.Xunit.Hosting/Codebelt.Extensions.Xunit.Hosting.csproj:22-26
Timestamp: 2024-10-09T14:10:40.721Z
Learning: In the `src/Codebelt.Extensions.Xunit.Hosting/Codebelt.Extensions.Xunit.Hosting.csproj` file, only certain .NET 8 packages are available in version 8.0.1, while others remain at 8.0.0, so package versions may differ due to availability.
🪛 LanguageTool
README.md
[style] ~29-~29: The phrase ‘feel free to’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others and make it sound more professional.
Context: ...UTING.md) are welcome and appreciated. Feel free to submit issues, feature requests, or pul...
(FEEL_FREE_TO_STYLE_ME)
🔇 Additional comments (14)
test/Codebelt.Extensions.Xunit.Hosting.Tests/Codebelt.Extensions.Xunit.Hosting.Tests.csproj (2)
18-18: Consider impact of using preview package in tests
The Cuemon.Core package was previously using a preview version (9.0.0-preview.12). While preview packages are acceptable for development, they might introduce instability in tests. Please ensure this is intentional and that the version in Directory.Packages.props is appropriate for testing.
#!/bin/bash
# Description: Check if other test projects use preview packages
# Search for preview package references in test projects
echo "Checking test projects for preview package usage..."
fd -p "test/.*\.csproj$" -x rg "preview" {}18-19: Verify package versions in Directory.Packages.props
The version numbers have been correctly removed from PackageReference elements as part of centralizing package management. Let's verify the setup is complete.
✅ Verification successful
Package versions are correctly centralized
The verification shows that:
- Directory.Packages.props exists and is properly configured with
ManagePackageVersionsCentrallyset to true - Both packages are defined in the central file:
- Cuemon.Core: Version "9.0.0-rc.1"
- Xunit.Priority: Version "1.1.6"
- The project file correctly references these packages without version numbers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Directory.Packages.props exists and contains the required package versions
# Check if Directory.Packages.props exists and contains the package versions
echo "Checking Directory.Packages.props for package versions..."
rg -A 1 "PackageVersion.*Cuemon\.Core|PackageVersion.*Xunit\.Priority" Directory.Packages.props
# List all .csproj files to ensure consistent package reference style
echo -e "\nChecking all .csproj files for versioned package references..."
fd -e csproj -x rg "PackageReference.*Version="
Length of output: 158
Script:
#!/bin/bash
# Let's first check if Directory.Packages.props exists
if [ -f "Directory.Packages.props" ]; then
echo "Directory.Packages.props exists"
cat Directory.Packages.props
else
echo "Directory.Packages.props does not exist"
fi
# Let's also check if there might be a different central package management file
echo -e "\nChecking for other potential central package management files:"
fd "Directory\.Build\.props|Directory\.Packages\.props|Packages\.props" --type f
# Let's examine the specific project file to see its current state
echo -e "\nExamining the test project file:"
cat "test/Codebelt.Extensions.Xunit.Hosting.Tests/Codebelt.Extensions.Xunit.Hosting.Tests.csproj"
Length of output: 4583
src/Codebelt.Extensions.Xunit/Codebelt.Extensions.Xunit.csproj (2)
20-21: Verify compatibility packages for .NET 8/9 upgrade
Since the PR involves upgrading to support .NET 8/9, we should verify that these compatibility packages (Microsoft.Bcl.AsyncInterfaces and System.Threading.Tasks.Extensions) are still required for netstandard2 and that their versions in Directory.Packages.props are compatible with .NET 8/9.
#!/bin/bash
# Description: Check package versions and framework compatibility
# Expected: Find version definitions and any framework-specific conditions
echo "Checking Directory.Packages.props for BCL package versions..."
rg -A 1 "PackageVersion.*(Bcl\.AsyncInterfaces|Threading\.Tasks\.Extensions)" Directory.Packages.props
echo "Checking for framework-specific conditions..."
rg "TargetFramework.*netstandard2" Directory.Build.props14-16: Verify xUnit package versions in Directory.Packages.props
The removal of version numbers aligns with the centralized package management approach. However, we should verify that these critical xUnit dependencies are properly defined in Directory.Packages.props.
✅ Verification successful
✓ xUnit package versions are properly defined in Directory.Packages.props
All required xUnit packages (xunit.assert, xunit.abstractions, and xunit.extensibility.core) have their versions correctly defined in the centralized package management file:
- xunit.assert: 2.9.2
- xunit.abstractions: 2.0.3
- xunit.extensibility.core: 2.9.2
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify xUnit package versions are defined in Directory.Packages.props
# Expected: Find version definitions for all xUnit packages
echo "Checking Directory.Packages.props for xUnit package versions..."
rg -A 1 "PackageVersion.*xunit" Directory.Packages.props || echo "No xUnit package versions found!"
Length of output: 774
Script:
#!/bin/bash
# Let's specifically check for xunit package versions
rg -A 1 "PackageVersion.*[Xx]unit" Directory.Packages.props
Length of output: 623
src/Codebelt.Extensions.Xunit.Hosting.AspNetCore/Codebelt.Extensions.Xunit.Hosting.AspNetCore.csproj (1)
18-19: LGTM! Verify package versions in Directory.Packages.props
The simplified package reference aligns with the PR objective of centralizing package version management.
Let's verify the package versions are properly defined in the centralized location:
✅ Verification successful
Package versions are properly defined in Directory.Packages.props
The verification confirms that the package versions are correctly defined in the centralized Directory.Packages.props:
- net9.0: Microsoft.AspNetCore.TestHost Version="9.0.0-rc.2.24474.3"
- net8.0: Microsoft.AspNetCore.TestHost Version="8.0.10"
This aligns with the multi-targeting requirements and maintains framework-specific versions through centralized management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify package versions are properly defined in Directory.Packages.props
# and check for any potential version conflicts
# Check if Directory.Packages.props exists and contains the package version
cat Directory.Packages.props | grep -A 5 "Microsoft.AspNetCore.TestHost"
# Look for any other references to this package that might conflict
rg "Microsoft.AspNetCore.TestHost.*Version" --type xml
Length of output: 1165
src/Codebelt.Extensions.Xunit.Hosting/Codebelt.Extensions.Xunit.Hosting.csproj (2)
13-18: Consider package version compatibility
Based on previous feedback about .NET package version differences, we should verify compatibility of these Microsoft.Extensions.* packages.
#!/bin/bash
# Description: Check package version compatibility in Directory.Packages.props
# Extract and compare versions for Microsoft.Extensions.* packages
echo "Checking Microsoft.Extensions.* package versions..."
rg "Microsoft\.Extensions\." Directory.Packages.props | sort
# Check if any conditional framework-specific versions exist
echo -e "\nChecking for framework-specific conditions..."
rg "Condition.*TargetFramework" Directory.Packages.props13-18: Verify centralized package version management
The removal of version attributes aligns with the PR objective of centralizing package versions. However, we should verify the proper setup of version management.
✅ Verification successful
✅ Package version centralization properly implemented
The verification confirms:
- Directory.Packages.props exists and contains all required Microsoft.Extensions.* package versions
- Framework-specific conditions are properly set up for net8, net9, and netstandard2
- No version attributes remain in the project file, adhering to centralized version management
- Package versions are correctly defined for all target frameworks (2.1.1, 8.0.0/8.0.1, and 9.0.0-rc.2)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Directory.Packages.props exists and contains required package versions
# Check if Directory.Packages.props exists and contains package versions
echo "Checking Directory.Packages.props for package versions..."
rg -A 1 "PackageVersion" Directory.Packages.props
# List all .csproj files to ensure consistent approach across projects
echo -e "\nChecking other project files for version attributes..."
fd -e csproj | xargs rg "PackageReference.*Version="
Length of output: 2115
Script:
#!/bin/bash
# Check the target frameworks in the project file
echo "Checking target frameworks in the project file..."
rg "TargetFramework" src/Codebelt.Extensions.Xunit.Hosting/Codebelt.Extensions.Xunit.Hosting.csproj
# Check if there are any framework-specific conditions in the project file
echo -e "\nChecking for framework-specific conditions..."
rg "Condition.*TargetFramework" src/Codebelt.Extensions.Xunit.Hosting/Codebelt.Extensions.Xunit.Hosting.csproj
# Check if there are any remaining version attributes in the project file
echo -e "\nVerifying no version attributes remain..."
rg "Version=" src/Codebelt.Extensions.Xunit.Hosting/Codebelt.Extensions.Xunit.Hosting.csproj
Length of output: 620
Directory.Packages.props (3)
1-4: LGTM! Good practice implementing central package management.
The implementation of centralized package version management is a recommended approach that will help maintain consistency across the project.
25-47: Consider updating .NET 8 packages to latest stable versions.
The framework-specific package versioning strategy looks good overall, with appropriate RC versions for .NET 9 and stable versions for .NET 8 and .NET Standard 2.x. However:
- Some .NET 8 packages might have newer stable versions available (current: 8.0.10)
- Consider documenting the rationale for keeping .NET Standard 2.x packages at version 2.1.1
Let's check for the latest stable versions of .NET 8 packages:
#!/bin/bash
# Description: Check latest stable versions for .NET 8 packages
echo "Checking latest stable versions for Microsoft.AspNetCore.TestHost..."
curl -s "https://api.nuget.org/v3-flatcontainer/Microsoft.AspNetCore.TestHost/index.json" | \
jq -r '.versions[]' | \
grep -E '^8\.0\.' | \
sort -V | \
tail -n 16-23: Verify package version compatibility and consider updates.
A few concerns about the package versions:
- Inconsistent xunit package versions: Most xunit packages are at 2.9.2, but xunit.runner.visualstudio is at 2.8.2. This might cause compatibility issues.
- Some packages appear outdated and might have important updates:
- NativeLibraryLoader (1.0.13)
- System.Threading.Tasks.Extensions (4.5.4)
Let's verify the latest stable versions and any security advisories:
Directory.Build.props (2)
72-82: Verify xUnit package version compatibility
The removal of explicit versions is aligned with centralized package management. However, we should ensure compatible versions are defined in Directory.Packages.props, as xUnit packages often need to be in sync.
#!/bin/bash
# Description: Check xUnit package versions in Directory.Packages.props
# Expected: Compatible versions for all xUnit packages
if [ -f "Directory.Packages.props" ]; then
echo "Checking xUnit package versions in Directory.Packages.props:"
cat Directory.Packages.props | grep -A 1 -E "xunit|Microsoft\.NET\.Test\.Sdk"
# Check if any test projects override these versions
echo -e "\nChecking for version overrides in test projects:"
fd -e csproj | xargs grep -l "Tests" | xargs grep -l "PackageReference.*xunit"
fiConsider adding a comment in Directory.Packages.props documenting the version compatibility requirements between xUnit packages to prevent future mismatches.
45-46: Verify build tool versions in Directory.Packages.props
The version-less package references for build tools look good, but we should verify they are properly defined in the centralized package management file.
✅ Verification successful
The previous output showed a partial match for Microsoft.SourceLink.GitHub but was filtered for xUnit packages. Let's get a clean verification of both build tool versions.
✓ Build tool versions are properly defined
Both build tools have their versions correctly specified in Directory.Packages.props:
- Microsoft.SourceLink.GitHub: 8.0.0
- MinVer: 6.0.0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if build tool versions are defined in Directory.Packages.props
# Expected: Version entries for Microsoft.SourceLink.GitHub and MinVer
if [ -f "Directory.Packages.props" ]; then
echo "Checking build tool versions in Directory.Packages.props:"
cat Directory.Packages.props | grep -A 1 -E "Microsoft\.SourceLink\.GitHub|MinVer"
else
echo "Warning: Directory.Packages.props not found"
fi
Length of output: 1022
Script:
#!/bin/bash
# Let's check Directory.Packages.props specifically for our build tools
cat Directory.Packages.props | grep -A 1 -B 1 'PackageVersion.*\(Microsoft\.SourceLink\.GitHub\|MinVer\)'
Length of output: 387
test/Codebelt.Extensions.Xunit.Tests/Assets/UnmanagedDisposable.cs (1)
Line range hint 31-89: Document or implement the Linux test scenario
The comment "i don't know of any native methods on unix" suggests incomplete implementation. This should be properly documented or implemented.
Let's check if there are any tests that actually run this code on Linux:
Consider implementing a proper Linux test scenario using common libc functions like open() or document why it's not necessary.
CHANGELOG.md (1)
10-10: LGTM! Well-structured changelog entry.
The release date follows the ISO 8601 format and aligns with the planned v9.0.0 release. The changelog provides comprehensive details about breaking changes and migration paths, which is excellent for maintainability.
This pull request includes several updates to package references, as well as some documentation and project configuration changes. The most important changes include updating package versions, centralizing package version management, and modifying conditional package references.
Package Management:
Directory.Packages.propsfile with defined package versions.PackageReferenceelements inDirectory.Build.propsand individual project files to rely on the centralized package versions. [1] [2] [3] [4] [5] [6] [7] [8]Documentation:
CHANGELOG.mdto 2024-11-13.README.mdby adding a link to theCONTRIBUTING.mdfile.Code Adjustments:
UnmanagedDisposable.csto target .NET 8.0 or greater instead of .NET 6.0 or greater. [1] [2]Summary by CodeRabbit
Release Notes for Version 9.0.0
New Features
StringExtensionsclass with a method for replacing line endings.Changes
Bug Fixes
AspNetCoreHostFixtureclass.Documentation