-
Notifications
You must be signed in to change notification settings - Fork 57
Add unit tests #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add unit tests #15
Conversation
There was a problem hiding this 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.Testswith MSTest.Sdk 4.0.1 - Added null validation checks to
TaskPool,CollectionExtensions, andBase32classes - Enhanced stream validation in
BinaryNumberto 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.
TechnitiumLibrary.Tests/TechnitiumLibrary/IndependentTaskSchedulerTests.cs
Outdated
Show resolved
Hide resolved
TechnitiumLibrary.Tests/TechnitiumLibrary/IndependentTaskSchedulerTests.cs
Outdated
Show resolved
Hide resolved
…mplete before shutdown
|
I wrote tests for every major project except for the most important and largest one 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 |
There was a problem hiding this 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) |
Copilot
AI
Dec 7, 2025
There was a problem hiding this comment.
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.
| public bool IsTOTPValid(string totp, int windowSteps = 1) | |
| public bool IsTOTPValid(string totp, int windowSteps = 10) |
| if (KeyUri.Digits < 6 || KeyUri.Digits > 8) | ||
| throw new ArgumentOutOfRangeException(nameof(keyUri), "Digits should be 6–8 per common TOTP deployments."); |
Copilot
AI
Dec 7, 2025
There was a problem hiding this comment.
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.
| 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."); |
| { | ||
| private static (BinaryWriter writer, MemoryStream stream) CreateWriter() | ||
| { | ||
| var ms = new MemoryStream(); |
Copilot
AI
Dec 7, 2025
There was a problem hiding this comment.
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.
| private static (BinaryWriter writer, MemoryStream stream) CreateWriter() | ||
| { | ||
| var ms = new MemoryStream(); | ||
| var bw = new BinaryWriter(ms); |
Copilot
AI
Dec 7, 2025
There was a problem hiding this comment.
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.
|
|
||
| private static PackageItem Roundtrip(PackageItem source) | ||
| { | ||
| var buffer = new MemoryStream(); // do NOT dispose here |
Copilot
AI
Dec 7, 2025
There was a problem hiding this comment.
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.
| var buffer = new MemoryStream(); // do NOT dispose here | |
| using var buffer = new MemoryStream(); |
| Assert.ThrowsExactly<ObjectDisposedException>(() => { var _ = s1.Length; }); | ||
| Assert.ThrowsExactly<ObjectDisposedException>(() => { var _ = s2.Length; }); |
Copilot
AI
Dec 7, 2025
There was a problem hiding this comment.
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.
| 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; }); |
| Assert.ThrowsExactly<ObjectDisposedException>(() => { var _ = s1.Length; }); | ||
| Assert.ThrowsExactly<ObjectDisposedException>(() => { var _ = s2.Length; }); |
Copilot
AI
Dec 7, 2025
There was a problem hiding this comment.
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.
| 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; }); |
|
|
||
| Assert.ThrowsExactly<IOException>(() => | ||
| { | ||
| var _ = PackageItem.Parse(buffer); |
Copilot
AI
Dec 7, 2025
There was a problem hiding this comment.
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.
| var _ = PackageItem.Parse(buffer); | |
| PackageItem.Parse(buffer); |
|
|
||
| Assert.ThrowsExactly<IOException>(() => | ||
| { | ||
| var _ = pkg.Items; |
Copilot
AI
Dec 7, 2025
There was a problem hiding this comment.
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.
| var _ = pkg.Items; | |
| pkg.Items; |
| { | ||
| HMAC hmac = null; | ||
| try | ||
| HMAC hmac = algorithm.ToUpperInvariant() switch |
Copilot
AI
Dec 7, 2025
There was a problem hiding this comment.
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.
Adding unit tests for the library