Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .docfx/api/namespaces/Codebelt.Extensions.Xunit.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ Complements: [xUnit: Capturing Output](https://xunit.net/docs/capturing-output)
|Type|Ext|Methods|
|--:|:-:|---|
|ITestOutputHelper|⬇️|`WriteLines`|
|String|⬇️|`ReplaceLineEndings` (TFM netstandard2.0)|
4 changes: 2 additions & 2 deletions Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@

<ItemGroup Condition="'$(IsTestProject)' == 'true'">
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.11.1" />
<PackageReference Include="xunit" Version="2.9.1" />
<PackageReference Include="xunit.runner.console" Version="2.9.1" />
<PackageReference Include="xunit" Version="2.9.2" />
<PackageReference Include="xunit.runner.console" Version="2.9.2" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.8.2" />
<PackageReference Include="coverlet.msbuild" Version="6.0.2">
<PrivateAssets>all</PrivateAssets>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="Cuemon.Extensions.DependencyInjection" Version="9.0.0-preview.8" />
<PackageReference Include="Cuemon.IO" Version="9.0.0-preview.8" />
<PackageReference Include="Cuemon.Extensions.DependencyInjection" Version="9.0.0-preview.9" />
<PackageReference Include="Cuemon.IO" Version="9.0.0-preview.9" />
</ItemGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="xunit.extensibility.core" Version="2.9.1" />
<PackageReference Include="xunit.extensibility.core" Version="2.9.2" />
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Found version discrepancies across Microsoft.Extensions packages.

Different target frameworks are using varying versions of Microsoft.Extensions packages:

  • net9: 9.0.0-rc.1.24431.7
  • net8: 8.0.0 and 8.0.1
  • netstandard2: 2.1.1

These inconsistencies may lead to compatibility and runtime issues. Please align the package versions across all target frameworks to ensure consistency and stability.

🔗 Analysis chain

Approve xunit.extensibility.core update with a compatibility check.

The update of xunit.extensibility.core from version 2.9.1 to 2.9.2 is a good practice to keep dependencies up-to-date. This minor version update likely includes bug fixes and small improvements.

To ensure compatibility, please verify that this update doesn't introduce any breaking changes by running the project's test suite. You can use the following command to check for any potential issues:

If any issues are found, please address them before merging this change.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential compatibility issues with the updated xunit.extensibility.core package

# Search for xunit-related test files
test_files=$(fd -e cs -e fs --type f --search-path ./tests)

# If test files are found, suggest running tests
if [ -n "$test_files" ]; then
    echo "Found potential test files. Please run your test suite to verify compatibility."
    echo "Test files:"
    echo "$test_files"
else
    echo "No test files found. Please ensure compatibility through manual testing or by reviewing the package's release notes."
fi

# Check for any explicit version references to xunit.extensibility.core in the code
rg "xunit\.extensibility\.core.*2\.9\.1" --type cs

Length of output: 6640

</ItemGroup>

<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
</PropertyGroup>

<PropertyGroup>
<Description>The Cuemon.Extensions.Xunit namespace contains types that provides a uniform way of doing unit testing. The namespace relates to the Xunit.Abstractions namespace.</Description>
<Description>The Codebelt.Extensions.Xunit namespace contains types that provides a uniform way of doing unit testing. The namespace relates to the Xunit.Abstractions namespace.</Description>
<PackageTags>test test-output test-disposable test-cleanup</PackageTags>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Cuemon.Core" Version="9.0.0-preview.8" />
<PackageReference Include="xunit.assert" Version="2.9.1" />
<PackageReference Include="Cuemon.Core" Version="9.0.0-preview.9" />
<PackageReference Include="xunit.assert" Version="2.9.2" />
<PackageReference Include="xunit.abstractions" Version="2.0.3" />
</ItemGroup>

Expand Down
8 changes: 8 additions & 0 deletions src/Codebelt.Extensions.Xunit/GlobalSuppressions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// This file is used by Code Analysis to maintain SuppressMessage
// attributes that are applied to this project.
// Project-level suppressions either have no target or are given
// a specific target and scoped to a namespace, type, member, etc.

using System.Diagnostics.CodeAnalysis;

[assembly: SuppressMessage("Major Code Smell", "S3881:\"IDisposable\" should be implemented correctly", Justification = "This is a base class implementation of the IDisposable interface tailored to avoid wrong implementations.", Scope = "type", Target = "~T:Codebelt.Extensions.Xunit.Test")]
44 changes: 40 additions & 4 deletions src/Codebelt.Extensions.Xunit/Test.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,8 @@ namespace Codebelt.Extensions.Xunit
/// <summary>
/// Represents the base class from which all implementations of unit testing should derive.
/// </summary>
/// <seealso cref="Disposable"/>
/// <seealso cref="ITestOutputHelper"/>
public abstract class Test : Disposable, ITest
public abstract class Test : ITest
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Explicitly implement IDisposable interface

The Test class now implements the Dispose() method, effectively making it disposable. To improve clarity and adhere to .NET conventions, consider explicitly implementing the IDisposable interface in the class declaration.

Apply this diff to update the class declaration:

-public abstract class Test : ITest
+public abstract class Test : ITest, IDisposable
📝 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 abstract class Test : ITest
public abstract class Test : ITest, IDisposable

{
/// <summary>
/// Provides a way, with wildcard support, to determine if <paramref name="actual" /> matches <paramref name="expected" />.
Expand Down Expand Up @@ -71,10 +70,47 @@ protected Test(ITestOutputHelper output = null, Type callerType = null)
protected bool HasTestOutput => TestOutput != null;

/// <summary>
/// Called when this object is being disposed by either <see cref="M:Cuemon.Disposable.Dispose" /> or <see cref="M:Cuemon.Disposable.Dispose(System.Boolean)" /> having <c>disposing</c> set to <c>true</c> and <see cref="P:Cuemon.Disposable.Disposed" /> is <c>false</c>.
/// Gets a value indicating whether this <see cref="Test"/> object is disposed.
/// </summary>
protected override void OnDisposeManagedResources()
/// <value><c>true</c> if this <see cref="Test"/> object is disposed; otherwise, <c>false</c>.</value>
public bool Disposed { get; private set; }
Comment on lines +73 to +76
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update XML documentation for Disposed property

The XML documentation for the Disposed property still references <see cref="Disposable"/>, which is no longer applicable since the class no longer inherits from Disposable. Update these comments to reflect the current class structure.

Apply this diff to correct the comments:

-/// Gets a value indicating whether this <see cref="Test"/> object is disposed.
+/// Gets a value indicating whether this object is disposed.

-/// <value><c>true</c> if this <see cref="Test"/> object is disposed; otherwise, <c>false</c>.</value>
+/// <value><c>true</c> if this object is disposed; otherwise, <c>false</c>.</value>
📝 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
/// Gets a value indicating whether this <see cref="Test"/> object is disposed.
/// </summary>
protected override void OnDisposeManagedResources()
/// <value><c>true</c> if this <see cref="Test"/> object is disposed; otherwise, <c>false</c>.</value>
public bool Disposed { get; private set; }
/// Gets a value indicating whether this object is disposed.
/// </summary>
/// <value><c>true</c> if this object is disposed; otherwise, <c>false</c>.</value>
public bool Disposed { get; private set; }


/// <summary>
/// Called when this object is being disposed by either <see cref="Dispose()" /> or <see cref="Dispose(bool)" /> having <c>disposing</c> set to <c>true</c> and <see cref="Disposed" /> is <c>false</c>.
/// </summary>
protected virtual void OnDisposeManagedResources()
{
}

/// <summary>
/// Called when this object is being disposed by either <see cref="Dispose()"/> or <see cref="Dispose(bool)"/> and <see cref="Disposed"/> is <c>false</c>.
/// </summary>
protected virtual void OnDisposeUnmanagedResources()
{
}

/// <summary>
/// Releases all resources used by the <see cref="Test"/> object.
/// </summary>
Comment on lines +93 to +94
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update XML documentation for Dispose methods

The XML documentation for both Dispose() and Dispose(bool disposing) methods still reference <see cref="Disposable"/>, which is no longer applicable. Update these comments to reflect the current class structure.

Apply these diffs to correct the comments:

For Dispose():

-/// Releases all resources used by the <see cref="Test"/> object.
+/// Releases all resources used by the object.

For Dispose(bool disposing):

-/// Releases the unmanaged resources used by the <see cref="Test"/> object and optionally releases the managed resources.
+/// Releases the unmanaged resources used by the object and optionally releases the managed resources.

Also applies to: 102-104

public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Evaluate the necessity of calling GC.SuppressFinalize(this);

Since the Test class does not declare a finalizer, calling GC.SuppressFinalize(this); is unnecessary. This method suppresses finalization, which is only relevant if a finalizer exists. Consider removing this call to adhere to the .NET dispose pattern.

Apply this diff:

Dispose(true);
-GC.SuppressFinalize(this);
📝 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
GC.SuppressFinalize(this);
Dispose(true);

}
Comment on lines +95 to +99
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove unnecessary GC.SuppressFinalize call

Since the Test class does not declare a finalizer, calling GC.SuppressFinalize(this); is unnecessary. This method suppresses finalization, which is only relevant if a finalizer exists.

Apply this diff to remove the unnecessary call:

public void Dispose()
{
    Dispose(true);
-   GC.SuppressFinalize(this);
}
📝 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 void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}
public void Dispose()
{
Dispose(true);
}


/// <summary>
/// Releases the unmanaged resources used by the <see cref="Test"/> object and optionally releases the managed resources.
/// </summary>
/// <param name="disposing"><c>true</c> to release both managed and unmanaged resources; <c>false</c> to release only unmanaged resources.</param>
protected void Dispose(bool disposing)
{
if (Disposed) { return; }
if (disposing)
{
OnDisposeManagedResources();
}
OnDisposeUnmanagedResources();
Disposed = true;
Comment on lines +105 to +113
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider making Dispose(bool disposing) method virtual

To allow derived classes to override the disposal logic, the Dispose(bool disposing) method should be declared as protected virtual. This aligns with the standard dispose pattern for unsealed classes and facilitates proper resource management in subclasses.

Apply this diff:

-protected void Dispose(bool disposing)
+protected virtual void Dispose(bool disposing)
📝 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 void Dispose(bool disposing)
{
if (Disposed) { return; }
if (disposing)
{
OnDisposeManagedResources();
}
OnDisposeUnmanagedResources();
Disposed = true;
protected virtual void Dispose(bool disposing)
{
if (Disposed) { return; }
if (disposing)
{
OnDisposeManagedResources();
}
OnDisposeUnmanagedResources();
Disposed = true;

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Cuemon.AspNetCore" Version="9.0.0-preview.8" />
<PackageReference Include="Cuemon.Extensions.AspNetCore" Version="9.0.0-preview.8" />
<PackageReference Include="Cuemon.Extensions.IO" Version="9.0.0-preview.8" />
<PackageReference Include="Cuemon.AspNetCore" Version="9.0.0-preview.9" />
<PackageReference Include="Cuemon.Extensions.AspNetCore" Version="9.0.0-preview.9" />
<PackageReference Include="Cuemon.Extensions.IO" Version="9.0.0-preview.9" />
</ItemGroup>

<ItemGroup>
Expand Down
26 changes: 26 additions & 0 deletions test/Codebelt.Extensions.Xunit.Tests/Assets/ManagedDisposable.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
using System.IO;

namespace Codebelt.Extensions.Xunit.Assets
{
public class ManagedDisposable : Test
{
public ManagedDisposable()
{
Stream = new MemoryStream();
}

public MemoryStream Stream { get; private set; }

protected override void OnDisposeManagedResources()
{
try
{
Stream?.Dispose();
}
finally
{
Stream = null;
}
}
}
}
128 changes: 128 additions & 0 deletions test/Codebelt.Extensions.Xunit.Tests/Assets/UnmanagedDisposable.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
using System;
using System.Runtime.InteropServices;
#if NET48_OR_GREATER
using NativeLibraryLoader;
#endif
Comment on lines +3 to +5
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct the conditional compilation symbols

The symbol NET48_OR_GREATER is not a standard predefined symbol. Typically, .NET version symbols are like NETFRAMEWORK, NETCOREAPP, or NETSTANDARD. Ensuring the correct symbols are used is crucial for the intended code paths during compilation.

Update the conditional compilation directives to use the correct symbols:

- #if NET48_OR_GREATER
+ #if NETFRAMEWORK
  using NativeLibraryLoader;
  #endif

Similarly, adjust other directives to match the appropriate target frameworks.

Also applies to: 26-28

namespace Codebelt.Extensions.Xunit.Assets
{
public class UnmanagedDisposable : Test
{
internal IntPtr _handle = IntPtr.Zero;
internal IntPtr _libHandle = IntPtr.Zero;

public delegate bool CloseHandle(IntPtr hObject);

public delegate IntPtr CreateFileDelegate(string lpFileName,
uint dwDesiredAccess,
uint dwShareMode,
IntPtr lpSecurityAttributes,
uint dwCreationDisposition,
uint dwFlagsAndAttributes,
IntPtr hTemplateFile);

public delegate IntPtr PtSname(int fd);

#if NET48_OR_GREATER
internal NativeLibrary _nativeLibrary;
#endif

public UnmanagedDisposable()
{
#if NET6_0_OR_GREATER
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
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",
0x80000000, //access read-only
1, //share-read
IntPtr.Zero,
3, //open existing
0,
IntPtr.Zero);
}
}
}
else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
if (NativeLibrary.TryLoad("libc.so.6", GetType().Assembly, DllImportSearchPath.SafeDirectories, out _libHandle))
{
_handle = _libHandle; // i don't know of any native methods on unix
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Review the assignment _handle = _libHandle on Unix platforms

Assigning _handle = _libHandle may not be meaningful, as _libHandle represents the handle to the loaded native library, not a separate unmanaged resource that requires disposal through _handle. This could lead to confusion or improper resource management during disposal.

Consider removing this assignment or properly initializing _handle with a valid unmanaged resource on Unix platforms. If there are no unmanaged resources to handle, you might want to adjust the logic accordingly.

Also applies to: 76-76

}
}
#else
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
_nativeLibrary = new NativeLibrary("kernel32.dll");
_libHandle = _nativeLibrary.Handle;
var functionHandle = _nativeLibrary.LoadFunction("CreateFileW");
var createFileFunc = Marshal.GetDelegateForFunctionPointer<CreateFileDelegate>(functionHandle);
_handle = createFileFunc(@"C:\TestFile.txt",
0x80000000, //access read-only
1, //share-read
IntPtr.Zero,
3, //open existing
0,
IntPtr.Zero);
}
else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
_nativeLibrary = new NativeLibrary("libc.so.6");
_libHandle = _nativeLibrary.Handle;
_handle = _libHandle; // i don't know of any native methods on unix
}
#endif
}

~UnmanagedDisposable()
{
Dispose(false);
}


protected override void OnDisposeManagedResources()
{

}

protected override void OnDisposeUnmanagedResources()
{
#if NET6_0_OR_GREATER
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
if (_handle != IntPtr.Zero)
{
if (NativeLibrary.TryGetExport(_libHandle, "CloseHandle", out var closeHandle))
{
var closeHandleAction = Marshal.GetDelegateForFunctionPointer<CloseHandle>(closeHandle);
closeHandleAction(_handle);
}
}
NativeLibrary.Free(_libHandle);
}
else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
NativeLibrary.Free(_libHandle);
}
#else
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
if (_handle != IntPtr.Zero)
{
var closeHandle = _nativeLibrary.LoadFunction("CloseHandle");
var closeHandleAction = Marshal.GetDelegateForFunctionPointer<CloseHandle>(closeHandle);
closeHandleAction(_handle);
}
_nativeLibrary.Dispose();
}
else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
_nativeLibrary.Dispose();
}
#endif
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,8 @@
<RootNamespace>Codebelt.Extensions.Xunit</RootNamespace>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="NativeLibraryLoader" Version="1.0.13" />
</ItemGroup>

</Project>
61 changes: 61 additions & 0 deletions test/Codebelt.Extensions.Xunit.Tests/DisposableTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
using System;
using Codebelt.Extensions.Xunit.Assets;
using Xunit;
using Xunit.Abstractions;

namespace Codebelt.Extensions.Xunit
{
public class DisposableTest : Test
{
public DisposableTest(ITestOutputHelper output) : base(output)
{
}



[Fact]
public void ManagedDisposable_VerifyThatAssetIsBeingDisposed()
{
ManagedDisposable mdRef = null;
using (var md = new ManagedDisposable())
{
mdRef = md;
Assert.NotNull(md.Stream);
Assert.Equal(0, md.Stream.Length);
Assert.False(mdRef.Disposed);
}
Assert.NotNull(mdRef);
Assert.Null(mdRef.Stream);
Assert.True(mdRef.Disposed);
}

private WeakReference<UnmanagedDisposable> unmanaged = null;

[Fact]
public void UnmanagedDisposable_VerifyThatAssetIsBeingDisposedOnFinalize()
{
Action body = () =>
{
var o = new UnmanagedDisposable();
Assert.NotEqual(IntPtr.Zero, o._libHandle);
Assert.NotEqual(IntPtr.Zero, o._handle);
unmanaged = new WeakReference<UnmanagedDisposable>(o, true);
};

try
{
body();
}
finally
{
GC.Collect(0, GCCollectionMode.Forced);
GC.WaitForPendingFinalizers();
}

if (unmanaged.TryGetTarget(out var ud2))
{
Assert.True(ud2.Disposed);
}
}
}
}
Comment on lines +1 to +61
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding more test cases for comprehensive coverage.

The DisposableTest class provides good basic coverage for both managed and unmanaged disposables. To further enhance the test suite, consider adding the following test cases:

  1. Test explicit disposal of UnmanagedDisposable using the IDisposable.Dispose() method.
  2. Test for proper handling of multiple dispose calls (should not throw exceptions).
  3. Test the behavior when accessing disposed objects (should throw ObjectDisposedException).
  4. If applicable, test any custom logic in the Dispose(bool disposing) method.

Here's an example of an additional test case for explicit disposal of UnmanagedDisposable:

[Fact]
public void UnmanagedDisposable_ExplicitDispose_VerifyResourcesAreReleased()
{
    var ud = new UnmanagedDisposable();
    Assert.NotEqual(IntPtr.Zero, ud._libHandle);
    Assert.NotEqual(IntPtr.Zero, ud._handle);
    
    ud.Dispose();
    
    Assert.True(ud.Disposed);
    Assert.Equal(IntPtr.Zero, ud._libHandle);
    Assert.Equal(IntPtr.Zero, ud._handle);
}

Adding these test cases will provide more comprehensive coverage and help ensure the robustness of your disposable implementations.