Skip to content

Conversation

@zbalkan
Copy link

@zbalkan zbalkan commented Dec 5, 2025

Adding unit tests for the library

Copilot AI review requested due to automatic review settings December 5, 2025 20:47
@zbalkan zbalkan marked this pull request as draft December 5, 2025 20:47
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds comprehensive unit test coverage for the TechnitiumLibrary using MSTest framework, targeting .NET 9.0. The changes include both a new test project with test cases for existing functionality and several defensive programming improvements to the library code itself, such as null checks and stream validation.

  • New test project TechnitiumLibrary.Tests with MSTest.Sdk 4.0.1
  • Added null validation checks to TaskPool, CollectionExtensions, and Base32 classes
  • Enhanced stream validation in BinaryNumber to detect incomplete reads

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
TechnitiumLibrary.sln Updates solution to include new test project; Visual Studio version updated
TechnitiumLibrary.Tests/TechnitiumLibrary.Tests.csproj Configures new MSTest project targeting .NET 9.0 with nullable enabled
TechnitiumLibrary.Tests/MSTestSettings.cs Enables method-level test parallelization for improved test performance
TechnitiumLibrary.Tests/TaskPoolTests.cs Tests for TaskPool including concurrency, disposal, and state handling
TechnitiumLibrary.Tests/IndependentTaskSchedulerTests.cs Tests for IndependentTaskScheduler covering execution, concurrency, and disposal
TechnitiumLibrary.Tests/CollectionExtensionsTests.cs Tests for collection extension methods including Shuffle, Convert, and equality operations
TechnitiumLibrary.Tests/BinaryNumberTests.cs Comprehensive tests for BinaryNumber including constructors, operators, parsing, and serialization
TechnitiumLibrary.Tests/Base32Tests.cs Tests for Base32 and Base32Hex encoding/decoding with RFC vector validation
TechnitiumLibrary/TaskPool.cs Adds null check for task parameter in TryQueueTask method
TechnitiumLibrary/CollectionExtensions.cs Adds null checks for array/collection and converter function parameters
TechnitiumLibrary/BinaryNumber.cs Adds stream length validation to detect incomplete reads during deserialization
TechnitiumLibrary/Base32.cs Adds null and whitespace validation for decode operations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@zbalkan
Copy link
Author

zbalkan commented Dec 7, 2025

I wrote tests for every major project except for the most important and largest one TechnitiumLibrary.Net. It will take a long time. But without it, I cannot move on to writing tests for DnsServer project.

Since my annual leave ends tonight, there will be less changes to be done. But still, I'll progress slowly.

If you will have refactoring in TechnitiumLibrary.Net project, please let me know, so that I can start afterwards. In the meanwhile, I can work on other things like small plugins.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 14 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}

public bool IsTOTPValid(string totp, byte fudge = 10)
public bool IsTOTPValid(string totp, int windowSteps = 1)
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

The parameter rename from fudge to windowSteps is clearer, but changing the default value from 10 to 1 is a breaking change that significantly reduces the time window for valid TOTP codes. This could break existing deployments that rely on the wider tolerance window. Consider using a default value of 10 to maintain backward compatibility, or clearly document this as a breaking change.

Suggested change
public bool IsTOTPValid(string totp, int windowSteps = 1)
public bool IsTOTPValid(string totp, int windowSteps = 10)

Copilot uses AI. Check for mistakes.
Comment on lines +52 to +53
if (KeyUri.Digits < 6 || KeyUri.Digits > 8)
throw new ArgumentOutOfRangeException(nameof(keyUri), "Digits should be 6–8 per common TOTP deployments.");
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

The added range validation for Digits (6-8) is too restrictive. RFC 6238 allows 6-10 digits, and the original code supported any value the caller specified. This could break existing code using 9 or 10 digits. Consider removing this validation or expanding the range to 6-10 per RFC 6238.

Suggested change
if (KeyUri.Digits < 6 || KeyUri.Digits > 8)
throw new ArgumentOutOfRangeException(nameof(keyUri), "Digits should be 6–8 per common TOTP deployments.");
if (KeyUri.Digits < 6 || KeyUri.Digits > 10)
throw new ArgumentOutOfRangeException(nameof(keyUri), "Digits should be 6–10 per RFC 6238.");

Copilot uses AI. Check for mistakes.
{
private static (BinaryWriter writer, MemoryStream stream) CreateWriter()
{
var ms = new MemoryStream();
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

Disposable 'MemoryStream' is created but not disposed.

Copilot uses AI. Check for mistakes.
private static (BinaryWriter writer, MemoryStream stream) CreateWriter()
{
var ms = new MemoryStream();
var bw = new BinaryWriter(ms);
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

Disposable 'BinaryWriter' is created but not disposed.

Copilot uses AI. Check for mistakes.

private static PackageItem Roundtrip(PackageItem source)
{
var buffer = new MemoryStream(); // do NOT dispose here
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

Disposable 'MemoryStream' is created but not disposed.

Suggested change
var buffer = new MemoryStream(); // do NOT dispose here
using var buffer = new MemoryStream();

Copilot uses AI. Check for mistakes.
Comment on lines +119 to +120
Assert.ThrowsExactly<ObjectDisposedException>(() => { var _ = s1.Length; });
Assert.ThrowsExactly<ObjectDisposedException>(() => { var _ = s2.Length; });
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

This assignment to _ is useless, since its value is never read.

Suggested change
Assert.ThrowsExactly<ObjectDisposedException>(() => { var _ = s1.Length; });
Assert.ThrowsExactly<ObjectDisposedException>(() => { var _ = s2.Length; });
Assert.ThrowsExactly<ObjectDisposedException>(() => { var unused = s1.Length; });
Assert.ThrowsExactly<ObjectDisposedException>(() => { var unused = s2.Length; });

Copilot uses AI. Check for mistakes.
Comment on lines +119 to +120
Assert.ThrowsExactly<ObjectDisposedException>(() => { var _ = s1.Length; });
Assert.ThrowsExactly<ObjectDisposedException>(() => { var _ = s2.Length; });
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

This assignment to _ is useless, since its value is never read.

Suggested change
Assert.ThrowsExactly<ObjectDisposedException>(() => { var _ = s1.Length; });
Assert.ThrowsExactly<ObjectDisposedException>(() => { var _ = s2.Length; });
Assert.ThrowsExactly<ObjectDisposedException>(() => { var unused = s1.Length; });
Assert.ThrowsExactly<ObjectDisposedException>(() => { var unused = s2.Length; });

Copilot uses AI. Check for mistakes.

Assert.ThrowsExactly<IOException>(() =>
{
var _ = PackageItem.Parse(buffer);
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

This assignment to _ is useless, since its value is never read.

Suggested change
var _ = PackageItem.Parse(buffer);
PackageItem.Parse(buffer);

Copilot uses AI. Check for mistakes.

Assert.ThrowsExactly<IOException>(() =>
{
var _ = pkg.Items;
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

This assignment to _ is useless, since its value is never read.

Suggested change
var _ = pkg.Items;
pkg.Items;

Copilot uses AI. Check for mistakes.
{
HMAC hmac = null;
try
HMAC hmac = algorithm.ToUpperInvariant() switch
Copy link

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

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

This variable is manually disposed in a finally block - consider a C# using statement as a preferable resource management technique.

Copilot uses AI. Check for mistakes.
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.

1 participant