feat!: Enabled .NET 10 support #200
Conversation
samtrion
added
state:ready for merge
There was a problem hiding this comment.
Pull Request Overview
This PR enables .NET 10 target framework support for the NetEvolve.Extensions.Data library by updating the target frameworks in the build configuration to include net10.0 for both project and test assemblies, and updating the GitVersion target framework to .NET 10.
Key Changes
- Added
net10.0to the multi-targeting configuration for library projects and test projects - Updated GitVersion target framework from
net8.0tonet10.0
samtrion
force-pushed
the
feature/dotnet-10
branch
from
9128c0b to
085234f
Compare
tests/NetEvolve.Extensions.Data.Tests.Unit/DbDataReaderExtensionsTests.cs
Show resolved
Hide resolved
tests/NetEvolve.Extensions.Data.Tests.Integration/IDataReaderExtensionsIntegrationTests.cs
Show resolved
Hide resolved
tests/NetEvolve.Extensions.Data.Tests.Integration/IDataRecordExtensionsIntegrationTests.cs
Show resolved
Hide resolved
tests/NetEvolve.Extensions.Data.Tests.Integration/IDataRecordExtensionsIntegrationTests.cs
Show resolved
Hide resolved
tests/NetEvolve.Extensions.Data.Tests.Integration/IDataRecordExtensionsIntegrationTests.cs
Show resolved
Hide resolved
tests/NetEvolve.Extensions.Data.Tests.Integration/IDataRecordExtensionsIntegrationTests.cs
Show resolved
Hide resolved
tests/NetEvolve.Extensions.Data.Tests.Integration/IDataRecordExtensionsIntegrationTests.cs
Show resolved
Hide resolved
tests/NetEvolve.Extensions.Data.Tests.Integration/IDataRecordExtensionsIntegrationTests.cs
Show resolved
Hide resolved
tests/NetEvolve.Extensions.Data.Tests.Integration/IDataRecordExtensionsIntegrationTests.cs
Show resolved
Hide resolved
tests/NetEvolve.Extensions.Data.Tests.Unit/IDataReaderExtensionsTests.cs
Outdated
Show resolved
Hide resolved
|
Warning Rate limit exceeded@samtrion has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request migrates test projects from xUnit to TUnit, adds net10.0 to target frameworks, converts many tests to async Task with async ADO.NET calls, updates package dependencies and test project attributes, and adds the Changes
Sequence DiagramsequenceDiagram
participant TestsOld as xUnit-style
participant TestsNew as TUnit-style
rect rgb(245,250,255)
Note over TestsOld,TestsNew: Framework & runtime change
end
TestsOld->>TestsOld: [Fact]/[Theory], sync or sync-style async
TestsNew->>TestsNew: [Test], async Task methods, MethodDataSource
TestsOld->>DB: ExecuteReader(), Read()
DB-->>TestsOld: result
TestsNew->>DB: await ExecuteReaderAsync(), await ReadAsync()
DB-->>TestsNew: awaited result
TestsOld->>TestsOld: xUnit assertions
TestsNew->>TestsNew: async-aware assertions (Assert.That(...))
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (11)
tests/NetEvolve.Extensions.Data.Tests.Integration/IDataReaderExtensionsIntegrationTests.cs (1)
73-82: Fix the duplicate assertion on line 76.The duplicate assertion for
hasIdon lines 75 and 76 remains unresolved. Line 76 should asserthasNameinstead. This means thehasNamevariable is never actually tested.Apply this diff to fix the issue:
using (Assert.Multiple()) { _ = await Assert.That(hasId).IsTrue(); - _ = await Assert.That(hasId).IsTrue(); + _ = await Assert.That(hasName).IsTrue(); - _ = await Assert.That(hasName).IsTrue(); _ = await Assert.That(hasEmail).IsTrue(); _ = await Assert.That(hasAge).IsTrue(); _ = await Assert.That(hasIsActive).IsTrue(); _ = await Assert.That(hasCreatedDate).IsTrue(); }tests/NetEvolve.Extensions.Data.Tests.Unit/IDataReaderExtensionsTests.cs (3)
16-16: Remove unnecessary assignment to discarded variable.The assignment to
_is unnecessary since the return value is not used.Apply this diff:
- _ = Assert.Throws<ArgumentNullException>("reader", () => reader.HasColumn(name)); + Assert.Throws<ArgumentNullException>("reader", () => reader.HasColumn(name));
27-27: Remove unnecessary assignment to discarded variable.The assignment to
_is unnecessary since the return value is not used.Apply this diff:
- _ = Assert.Throws<ArgumentNullException>("name", () => reader.HasColumn(name)); + Assert.Throws<ArgumentNullException>("name", () => reader.HasColumn(name));
38-38: Remove unnecessary assignment to discarded variable.The assignment to
_is unnecessary since the return value is not used.Apply this diff:
- _ = Assert.Throws<ArgumentException>("name", () => reader.HasColumn(name)); + Assert.Throws<ArgumentException>("name", () => reader.HasColumn(name));tests/NetEvolve.Extensions.Data.Tests.Integration/IDataRecordExtensionsIntegrationTests.cs (6)
283-288: Fix nullable access within Assert.Multiple block.Within
Assert.Multiple(), assertions don't throw immediately but are collected and reported together. This meansAssert.NotNull(result)on line 285 won't prevent line 286 from executing ifresultis null, potentially causing aNullReferenceException.Apply this diff to safely access the nullable value:
- using (Assert.Multiple()) - { - Assert.NotNull(result); - _ = await Assert.That(Math.Abs(result.Value - 123.456) < 0.001).IsTrue(); - } + Assert.NotNull(result); + _ = await Assert.That(Math.Abs(result.Value - 123.456) < 0.001).IsTrue();Alternatively, use pattern matching:
- using (Assert.Multiple()) - { - Assert.NotNull(result); - _ = await Assert.That(Math.Abs(result.Value - 123.456) < 0.001).IsTrue(); - } + if (result is double value) + { + _ = await Assert.That(Math.Abs(value - 123.456) < 0.001).IsTrue(); + } + else + { + Assert.Fail("Expected non-null result for NullableDouble."); + }
300-305: Fix nullable access within Assert.Multiple block.Same issue as lines 283-288: accessing
result.ValueafterAssert.NotNull()withinAssert.Multiple()can throwNullReferenceExceptionsince assertions don't throw immediately in this context.Apply the same fix pattern - either move
Assert.NotNull()outside the Multiple block or use pattern matching to safely access the value.
317-322: Fix nullable access within Assert.Multiple block.Same unsafe nullable access pattern:
Assert.NotNull(result)on line 319 won't preventNullReferenceExceptionon line 320 within theAssert.Multiple()block.
334-339: Fix nullable access within Assert.Multiple block.Same unsafe nullable access pattern as previous occurrences.
653-671: Fix multiple nullable accesses within Assert.Multiple block.Lines 663 and 665 access
.Valueon nullable types afterAssert.NotNull()checks withinAssert.Multiple(), which can causeNullReferenceExceptionif the values are null.Apply this diff to fix both nullable accesses:
using (Assert.Multiple()) { _ = await Assert.That(boolean).IsTrue(); _ = await Assert.That(byteValue).IsEqualTo((byte)255); _ = await Assert.That(charValue).IsEqualTo('A'); _ = await Assert .That(dateTime) .IsEqualTo(DateTime.Parse("2023-01-15 10:30:00", CultureInfo.InvariantCulture)); _ = await Assert.That(decimalValue).IsEqualTo(12345.67m); - Assert.NotNull(doubleValue); - _ = await Assert.That(Math.Abs(doubleValue.Value - 123.456) < 0.001).IsTrue(); - Assert.NotNull(floatValue); - _ = await Assert.That(Math.Abs(floatValue.Value - 78.9f) < 0.1f).IsTrue(); + _ = await Assert.That(doubleValue.HasValue && Math.Abs(doubleValue.Value - 123.456) < 0.001).IsTrue(); + _ = await Assert.That(floatValue.HasValue && Math.Abs(floatValue.Value - 78.9f) < 0.1f).IsTrue(); _ = await Assert.That(guidValue).IsEqualTo(Guid.Parse("550e8400-e29b-41d4-a716-446655440000")); _ = await Assert.That(int16Value).IsEqualTo((short)32767); _ = await Assert.That(int32Value).IsEqualTo(2147483647); _ = await Assert.That(int64Value).IsEqualTo(9223372036854775807L); _ = await Assert.That(stringValue).IsEqualTo("Not Null"); }
705-721: Fix multiple nullable accesses within Assert.Multiple block.Lines 713 and 715 access
.Valueon nullable types afterAssert.NotNull()checks withinAssert.Multiple(), which can causeNullReferenceException.Apply the same fix as in the previous test - either move null checks outside the Multiple block or use
HasValuein the assertion conditions.Directory.Build.props (1)
18-18: Verify GitVersion supports .NET 10.The
GitVersionTargetFrameworkis being updated tonet10.0, but the comment on line 17 references a workaround for GitVersion. Confirm that GitVersion officially supports .NET 10, or consider keeping this at a supported framework version (e.g.,net9.0) until official support is available.Does GitVersion support .NET 10 target framework?
🧹 Nitpick comments (3)
tests/NetEvolve.Extensions.Data.Tests.Integration/DbDataReaderExtensionsIntegrationTests.cs (1)
166-169: Consider standardizing assertion style for consistency.These assertions use
Assert.Null()without await, while the rest of the file consistently uses theawait Assert.That(...).IsNull()pattern. For uniformity with TUnit best practices, consider converting these to the awaited pattern.Example for lines 166-169:
- Assert.Null(nullableInt); - Assert.Null(nullableText); - Assert.Null(nullableBoolean); - Assert.Null(nullableDouble); + _ = await Assert.That(nullableInt).IsNull(); + _ = await Assert.That(nullableText).IsNull(); + _ = await Assert.That(nullableBoolean).IsNull(); + _ = await Assert.That(nullableDouble).IsNull();Apply similar changes to lines 231-232, 268-271, and 332-333.
Also applies to: 231-232, 268-271, 332-333
tests/NetEvolve.Extensions.Data.Tests.Integration/IDataRecordExtensionsIntegrationTests.cs (1)
564-623: Consider simplifying exception test assertions.These exception handling tests are marked
async Taskand use async assertions for simple property checks (e.g.,await Assert.That(exception.ParamName).IsEqualTo("record")). Since the methods under test are synchronous and property access is not async, these tests could be simplified by using synchronous assertions.However, if the TUnit framework requires all tests to be async for consistency, the current pattern is acceptable.
tests/NetEvolve.Extensions.Data.Tests.Unit/IDataRecordExtensionsTests.cs (1)
19-28: Test names mention ArgumentOutOfRangeException but assertions expect IndexOutOfRangeExceptionFor example:
- Line 20:
GetNullableBooleanIndex_WhenIndexIsNegative_ThrowsArgumentOutOfRangeException- Line 27:
Assert.Throws<IndexOutOfRangeException>(...)- Line 78:
GetNullableBooleanNamed_WhenNameIsOrdinalNegative_ThrowsArgumentOutOfRangeException- Line 85:
Assert.Throws<IndexOutOfRangeException>(...)The method names advertise
ArgumentOutOfRangeException, but the assertions (and NSubstitute setup for negative ordinals) expectIndexOutOfRangeException. The same pattern appears in the corresponding tests for otherGetNullable*methods.To avoid confusion for future maintainers, consider either:
- Renaming the tests to
..._ThrowsIndexOutOfRangeException, or- Changing the assertions to expect
ArgumentOutOfRangeException(and adjusting the underlying implementation/stub accordingly, if that’s the desired contract).Example rename for one test (apply similarly to others if you choose this approach):
- public void GetNullableBooleanIndex_WhenIndexIsNegative_ThrowsArgumentOutOfRangeException() + public void GetNullableBooleanIndex_WhenIndexIsNegative_ThrowsIndexOutOfRangeException()Also applies to: 77-86
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (16)
.editorconfig(1 hunks)Directory.Build.props(1 hunks)Directory.Packages.props(1 hunks)tests/NetEvolve.Extensions.Data.Tests.Integration/DbDataReaderExtensionsIntegrationTests.cs(6 hunks)tests/NetEvolve.Extensions.Data.Tests.Integration/IDataReaderExtensionsIntegrationTests.cs(3 hunks)tests/NetEvolve.Extensions.Data.Tests.Integration/IDataRecordExtensionsIntegrationTests.cs(5 hunks)tests/NetEvolve.Extensions.Data.Tests.Integration/NetEvolve.Extensions.Data.Tests.Integration.csproj(1 hunks)tests/NetEvolve.Extensions.Data.Tests.PublicApi/NetEvolve.Extensions.Data.Tests.PublicApi.csproj(1 hunks)tests/NetEvolve.Extensions.Data.Tests.PublicApi/Predefined.cs(0 hunks)tests/NetEvolve.Extensions.Data.Tests.PublicApi/PublicApiTests.cs(1 hunks)tests/NetEvolve.Extensions.Data.Tests.PublicApi/_snapshots/DotNet10_0/PublicApiTests.PublicApi_HasNotChanged_Expected.verified.txt(1 hunks)tests/NetEvolve.Extensions.Data.Tests.Unit/DbDataReaderExtensionsTests.cs(20 hunks)tests/NetEvolve.Extensions.Data.Tests.Unit/GlobalUsings.cs(1 hunks)tests/NetEvolve.Extensions.Data.Tests.Unit/IDataReaderExtensionsTests.cs(2 hunks)tests/NetEvolve.Extensions.Data.Tests.Unit/IDataRecordExtensionsTests.cs(17 hunks)tests/NetEvolve.Extensions.Data.Tests.Unit/NetEvolve.Extensions.Data.Tests.Unit.csproj(1 hunks)
💤 Files with no reviewable changes (1)
- tests/NetEvolve.Extensions.Data.Tests.PublicApi/Predefined.cs
🧰 Additional context used
🪛 LanguageTool
tests/NetEvolve.Extensions.Data.Tests.PublicApi/_snapshots/DotNet10_0/PublicApiTests.PublicApi_HasNotChanged_Expected.verified.txt
[grammar] ~32-~32: Ensure spelling is correct
Context: ...fault) { } public static System.Guid? GetNullableGuid(this System.Data.IDataR...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~32-~32: Ensure spelling is correct
Context: ....Data.IDataRecord record, int i, System.Guid? defaultValue = default) { } pub...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~33-~33: Ensure spelling is correct
Context: ...fault) { } public static System.Guid? GetNullableGuid(this System.Data.IDataR...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~33-~33: Ensure spelling is correct
Context: ...IDataRecord record, string name, System.Guid? defaultValue = default) { } pub...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Tests / Run Tests / Testing .NET solution
- GitHub Check: Analyze (csharp)
🔇 Additional comments (33)
.editorconfig (1)
45-45: Verify this change bypasses the stated governance process.The file header (line 5) explicitly instructs contributors not to change settings directly but to "CREATE PULL REQUEST IN REPOSITORY
dotnet-engineering." However, this change to addslnxsupport was made directly here. While the change itself is technically sound and aligns with supporting Visual Studio's newer XML solution format, please confirm whether this should have been contributed to the centraldotnet-engineeringrepository instead.tests/NetEvolve.Extensions.Data.Tests.Integration/IDataReaderExtensionsIntegrationTests.cs (12)
4-4: LGTM: Import required for async tests.The
System.Threading.Tasksimport is correctly added to support the async Task method signatures.
85-100: LGTM: Async conversion correctly implemented.The test method has been properly converted to async Task pattern with correct await usage and assertions.
102-119: LGTM: Async conversion correctly implemented.The test method has been properly converted to async Task pattern with correct await usage and assertions.
121-142: LGTM: Async conversion correctly implemented.The test method has been properly converted to async Task pattern with correct await usage and assertions.
144-163: LGTM: Async conversion correctly implemented.The test method has been properly converted to async Task pattern with correct await usage and assertions.
165-182: LGTM: Async conversion correctly implemented.The test method has been properly converted to async Task pattern with correct await usage and assertions.
184-200: LGTM: Parameterized async test correctly implemented.The test method has been properly converted to TUnit's parameterized test pattern using
[Arguments]attributes and async Task signature.
202-215: LGTM: Parameterized async test correctly implemented.The test method has been properly converted to TUnit's parameterized test pattern using
[Arguments]attributes and async Task signature.
217-223: LGTM: Exception test correctly converted.The test method correctly remains synchronous and uses the appropriate Assert.Throws pattern for TUnit.
225-233: LGTM: Exception test correctly converted.The test method correctly remains synchronous and uses the appropriate Assert.Throws pattern for TUnit.
235-254: LGTM: Exception tests correctly converted.Both test methods correctly remain synchronous and use the appropriate Assert.Throws patterns for TUnit.
1-255: AI summary incorrectly identifies the test framework.The AI-generated summary states that the migration is from xUnit to "NUnit," but the code actually uses TUnit test framework conventions (e.g.,
[Test]with[Arguments]for parameterized tests). NUnit would use[TestCase]instead of[Arguments].tests/NetEvolve.Extensions.Data.Tests.Unit/IDataReaderExtensionsTests.cs (2)
43-43: Verify necessity of async test pattern.The test method is marked
async Taskand usesawait Assert.That(...), but the method under testHasColumnis called synchronously on line 54 without any async operations. This pattern adds unnecessary complexity unless TUnit specifically requires async assertions for parameterized tests.If TUnit supports synchronous assertions for parameterized tests, consider simplifying to:
- public async Task HasColumn_Theory_Expected(bool expected, string name) + public void HasColumn_Theory_Expected(bool expected, string name) { // Arrange var reader = Substitute.For<IDataReader>(); _ = reader.FieldCount.Returns(2); _ = reader.GetName(Arg.Is(0)).Returns("Id"); _ = reader.GetName(Arg.Is(1)).Returns("Name"); // Act var result = reader.HasColumn(name); // Assert - _ = await Assert.That(result).IsEqualTo(expected); + Assert.That(result).IsEqualTo(expected); }Also applies to: 57-57
60-60: LGTM!The migration from
TheoryData<bool, string>toIEnumerable<(bool, string)>is correct for TUnit. The test data appropriately covers case-insensitive matching and non-matching scenarios.tests/NetEvolve.Extensions.Data.Tests.Integration/DbDataReaderExtensionsIntegrationTests.cs (2)
4-4: LGTM! Suppressions are appropriate.The suppressions correctly justify the intentional mixing of synchronous extension methods (
GetFieldValue,GetFieldValueOrDefault) with asynchronous infrastructure methods (ExecuteReaderAsync,ReadAsync), as this test suite validates both sync and async API variants.Also applies to: 7-8
65-436: Excellent test coverage and migration execution.The async migration is well-executed with comprehensive coverage of:
- Synchronous and asynchronous extension methods
- Null handling and custom defaults
- Both column name and ordinal-based accessors
- Cancellation token support
- Exception scenarios
The test structure is clean, with proper resource management and clear test data setup.
tests/NetEvolve.Extensions.Data.Tests.Integration/IDataRecordExtensionsIntegrationTests.cs (1)
103-166: LGTM!The async conversion for these boolean test methods is correct, with proper use of
ExecuteReaderAsync,await reader.ReadAsync(), and TUnit assertion syntax.tests/NetEvolve.Extensions.Data.Tests.Unit/IDataRecordExtensionsTests.cs (2)
8-207: TUnit migration, async assertions, and MethodDataSource usage look consistentThe migration to
[Test]+[MethodDataSource]with asyncTasktests and_ = await Assert.That(result).IsEqualTo(expected);is consistent across allGetNullable*index/named theory methods. Data-source member names match thenameof(...)references, signatures align with the test parameters, and exception-based tests correctly useAssert.Throws<...>with the appropriate parameter names ("record","name"). I don’t see functional regressions in the testing logic from this refactor.Also applies to: 244-421, 465-649, 693-877, 921-1115, 1149-1457
102-113: Tuple-based data providers align with the NSubstitute stub in CreateTestRecordThe tuple
IEnumerable<(..., int/string, ...)>data providers for allGetNullable*tests line up with theCreateTestRecordstubbing:
- Ordinals and names (
Id→ 0 withIsDBNull(0) == true,Name→ 1,IsActive→ 2) are consistent across boolean, numeric, GUID, string, and object cases.- Default values in the data (e.g.,
falsevstrue,MinValue/MaxValue,Guid.Emptyvs_guidMax,string.Emptyvs"Hello World!") match the configuredGetXreturn values forArg.Any<int>vsArg.Is(1).- The “null” cases (index 0 /
"Id"withIsDBNull(0) == true) correctly expectnullwhendefaultValueisnulland the provided default when non-null.This gives good coverage of the extension behavior without introducing inconsistencies between the data sets and the stubbed record.
Also applies to: 209-221, 316-328, 423-441, 537-555, 651-669, 765-783, 879-897, 993-1011, 1107-1125, 1221-1239, 1335-1353, 1449-1467, 1471-1524
tests/NetEvolve.Extensions.Data.Tests.PublicApi/PublicApiTests.cs (1)
16-16: LGTM! Clean migration to TUnit.The attribute change from
[Fact]to[Test]correctly reflects the migration from xUnit to TUnit.tests/NetEvolve.Extensions.Data.Tests.Unit/GlobalUsings.cs (1)
2-4: LGTM! TUnit global usings correctly added.The global usings have been properly updated to support the TUnit testing framework.
tests/NetEvolve.Extensions.Data.Tests.Unit/DbDataReaderExtensionsTests.cs (3)
8-10: Appropriate use of SuppressMessage attributes.The suppression attributes correctly acknowledge that the async test method signatures are required by TUnit, even when the test body uses synchronous assertions. This is an intentional design pattern for TUnit migration.
13-20: Test migration follows TUnit conventions.The conversion of test methods to async Task with
[Test]attributes is consistent with TUnit framework requirements. The synchronousAssert.Throwscalls within async methods are intentionally suppressed (lines 8-10) as part of the framework migration pattern.Also applies to: 22-29, 31-38, 40-47
78-88: Data source migration to collection expressions is clean.The conversion from
TheoryData<...>toIEnumerable<(...)>with collection expression syntax ([...]) correctly aligns with TUnit's data source patterns and modern C# syntax.Also applies to: 173-183, 237-251, 342-350, 422-436, 550-558
tests/NetEvolve.Extensions.Data.Tests.PublicApi/_snapshots/DotNet10_0/PublicApiTests.PublicApi_HasNotChanged_Expected.verified.txt (1)
1-45: LGTM! .NET 10.0 public API snapshot correctly generated.The snapshot file accurately captures the public API surface for .NET 10.0 target framework. The static analysis hints about "Guid" spelling can be safely ignored as
System.Guidis the correct type name.Directory.Build.props (1)
12-13: LGTM! .NET 10.0 target frameworks added.The addition of
net10.0to both project and test target frameworks correctly enables .NET 10 support across the solution.tests/NetEvolve.Extensions.Data.Tests.PublicApi/NetEvolve.Extensions.Data.Tests.PublicApi.csproj (2)
8-12: LGTM! Test package references correctly migrated to TUnit.The package references have been properly updated from xUnit-based tooling to TUnit-based equivalents, including code coverage, test framework, and verification libraries.
18-18: LGTM! Assembly attribute updated for TUnit.The integration test attribute correctly references
NetEvolve.Extensions.TUnit.IntegrationTestAttribute, aligning with the framework migration.tests/NetEvolve.Extensions.Data.Tests.Unit/NetEvolve.Extensions.Data.Tests.Unit.csproj (2)
8-11: LGTM! Test project correctly configured for TUnit.The package references and assembly attribute have been properly updated to use TUnit-based testing infrastructure.
Also applies to: 17-17
5-5: Add inline documentation explaining TUnit0300 suppression.Investigation could not find TUnit0300 documented in the TUnit .NET framework—TUnit uses logging/diagnostic levels rather than numbered warning codes. However, the NoWarn directive in the project file suppresses this warning without any explanation.
Add a comment in the .csproj file clarifying: what TUnit0300 represents, why it's being suppressed, and when it can be removed. For example:
<!-- TUnit0300: [explanation of warning] - suppress because [reason] --> <NoWarn>$(NoWarn);TUnit0300;</NoWarn>This will help future maintainers understand the intentional suppression.
tests/NetEvolve.Extensions.Data.Tests.Integration/NetEvolve.Extensions.Data.Tests.Integration.csproj (2)
17-17: AssemblyAttribute update to TUnit is correct.Changing from
NetEvolve.Extensions.XUnit.IntegrationTestAttributetoNetEvolve.Extensions.TUnit.IntegrationTestAttributeis the appropriate way to activate integration test behavior in TUnit. Ensure all test methods in this project have been updated to use TUnit's async-first API and assertions.
7-11: Verify during build that all dependencies resolve correctly, especially Microsoft.Testing.Extensions.TrxReport.Migration from xUnit to TUnit is properly configured:
- ✓ All xUnit packages removed from the project file
- ✓ TUnit packages (TUnit 1.2.11, NetEvolve.Extensions.TUnit 2.7.59, Verify.TUnit 31.7.1) defined in Directory.Packages.props with compatible versions
- ✓ Microsoft.Testing.Extensions.CodeCoverage 18.1.0 present and compatible with net8.0/net9.0/net10.0 targets
- ✓ AssemblyAttribute correctly set to NetEvolve.Extensions.TUnit.IntegrationTestAttribute
- ✓ Target frameworks (net8.0; net9.0; net10.0) all supported by TUnit 1.2.11
One item requires verification: Microsoft.Testing.Extensions.TrxReport (>= 2.0.1) is listed as a peer dependency for TUnit 1.2.11 in the documentation, but is not explicitly defined in Directory.Packages.props. Confirm during a test run that it resolves as a transitive dependency or add it explicitly if build/restore fails.
tests/NetEvolve.Extensions.Data.Tests.Unit/IDataReaderExtensionsTests.cs
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Martin Stühmer <me@samtrion.net>
samtrion
force-pushed
the
feature/dotnet-10
branch
from
bcf9a36 to
a335f85
Compare
…207) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: samtrion <3283596+samtrion@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/NetEvolve.Extensions.Data.Tests.Unit/DbDataReaderExtensionsTests.cs (1)
247-247: Consider using fixed DateTime for test data consistency.The test data includes
DateTime.Nowon these lines. While this doesn't cause test failures (these values appear in rows whereisDBNull=true), using a fixed date like other test cases would improve consistency and eliminate non-deterministic values from test data.Apply this diff:
- (null, true, DateTime.Now), + (null, true, DateTime.Parse("2023-01-01", CultureInfo.InvariantCulture)),Also applies to: 436-436
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
tests/NetEvolve.Extensions.Data.Tests.Unit/DbDataReaderExtensionsTests.cs(16 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & Tests / Run Tests / Testing .NET solution
- GitHub Check: Analyze (csharp)
🔇 Additional comments (5)
tests/NetEvolve.Extensions.Data.Tests.Unit/DbDataReaderExtensionsTests.cs (5)
8-10: LGTM! SuppressMessage attributes are appropriately justified.The suppression attributes correctly acknowledge that test methods are async primarily to support TUnit's assertion API (
Assert.That), not because the methods under test are asynchronous. This is a valid framework requirement for TUnit.
13-47: LGTM! Synchronous tests correctly remain void.These exception-throwing tests appropriately remain synchronous (
void) since they useAssert.Throws, which doesn't require await. This addresses the concern in the past review comment.
78-88: LGTM! Test data sources correctly migrated to TUnit format.The data sources are properly converted from xUnit's
TheoryDatato TUnit'sIEnumerable<tuple>pattern with collection expression syntax. The tuple shapes correctly match the corresponding test method parameters.Also applies to: 175-185, 239-253, 344-352, 428-442, 556-564
49-76: LGTM! Async test methods correctly support TUnit's assertion API.These test methods are appropriately marked
async Taskto support TUnit'sAssert.ThatAPI, even though the methods under test are synchronous. This is a framework requirement and is properly covered by the SuppressMessage attributes.Also applies to: 208-237, 305-342
90-185: LGTM! Async tests correctly test async methods.All tests for async methods (
GetFieldValueAsync,GetFieldValueOrDefaultAsync) properly use async/await patterns withAssert.ThrowsAsyncfor exceptions andawait Assert.Thatfor value assertions. The implementation correctly tests async behavior including cancellation token support.Also applies to: 354-442, 444-564
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #200 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 114 114
Branches 30 28 -2
=========================================
Hits 114 114 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.