Skip to content

Improve test assertions and exception types for null unit validation#300

Merged
samsmithnz merged 2 commits intomainfrom
copilot/fix-exception-type-checks
Dec 4, 2025
Merged

Improve test assertions and exception types for null unit validation#300
samsmithnz merged 2 commits intomainfrom
copilot/fix-exception-type-checks

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Dec 4, 2025

Apply code review feedback: use specific exception types and cleaner assertions in tests.

Test Improvements

SkittleTests.cs:

  • Replace Assert.Throws<Exception> with Assert.Throws<ArgumentNullException> for null unit tests
  • Remove empty lines after //Act comments

UnitsTests.cs:

  • Rename UnitsTestsUnitTests (standard naming)
  • Assert.IsTrue(results != null)Assert.IsNotNull(results)
  • Assert.IsTrue(string.IsNullOrEmpty(x) == false)Assert.IsFalse(string.IsNullOrEmpty(x))

Core Code Fix

Updated GetCubicCmForRectangle and GetCubicCmForCylinder to throw ArgumentNullException on null/empty unit, matching existing pattern in GetCubicCmForVolume:

// Before: silent empty string assignment, falls through to generic Exception
if (string.IsNullOrEmpty(unit))
    unit = "";

// After: explicit ArgumentNullException
if (string.IsNullOrEmpty(unit))
    throw new ArgumentNullException(nameof(unit), "Unit cannot be null or empty.");

Note: CollectionAssert.IsNotEmpty from diff was skipped—not available in MSTest.

Original prompt
Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"Catching the generic Exception type is too broad. Consider asserting for a more specific exception type like ArgumentNullException to match the pattern used in line 112.","fixFiles":[{"filePath":"src/MandMCounter.Tests/SkittleTests.cs","diff":"diff --git a/src/MandMCounter.Tests/SkittleTests.cs b/src/MandMCounter.Tests/SkittleTests.cs\n--- a/src/MandMCounter.Tests/SkittleTests.cs\n+++ b/src/MandMCounter.Tests/SkittleTests.cs\n@@ -178,7 +178,7 @@\n             //Act\n \n             //Assert\n-            Assert.Throws<Exception>(() => Calculator.CountSkittles(unit, height, width, length));\n+            Assert.Throws<ArgumentNullException>(() => Calculator.CountSkittles(unit, height, width, length));\n         }\n \n         #endregion\n"}]},{"message":"Catching the generic Exception type is too broad. Consider asserting for a more specific exception type like ArgumentNullException to match the pattern used in line 112.","fixFiles":[{"filePath":"src/MandMCounter.Tests/SkittleTests.cs","diff":"diff --git a/src/MandMCounter.Tests/SkittleTests.cs b/src/MandMCounter.Tests/SkittleTests.cs\n--- a/src/MandMCounter.Tests/SkittleTests.cs\n+++ b/src/MandMCounter.Tests/SkittleTests.cs\n@@ -226,7 +226,7 @@\n             float radius = 5;\n \n             //Act & Assert\n-            Assert.Throws<Exception>(() => Calculator.CountSkittles(unit, height, radius));\n+            Assert.Throws<ArgumentNullException>(() => Calculator.CountSkittles(unit, height, radius));\n         }\n \n         #endregion\n"}]},{"message":"[nitpick] Empty line after //Act comment is unnecessary and appears consistently throughout the test file, creating visual clutter.","fixFiles":[{"filePath":"src/MandMCounter.Tests/SkittleTests.cs","diff":"diff --git a/src/MandMCounter.Tests/SkittleTests.cs b/src/MandMCounter.Tests/SkittleTests.cs\n--- a/src/MandMCounter.Tests/SkittleTests.cs\n+++ b/src/MandMCounter.Tests/SkittleTests.cs\n@@ -18,7 +18,6 @@\n             float quantity = 1f;\n \n             //Act\n-            \n             float result = Calculator.CountSkittles(unit, quantity);\n \n             //Assert\n@@ -33,7 +32,6 @@\n             float quantity = 1f;\n \n             //Act\n-            \n             float result = Calculator.CountSkittles(unit, quantity);\n \n             //Assert\n@@ -48,7 +46,6 @@\n             float quantity = 1f;\n \n             //Act\n-            \n             float result = Calculator.CountSkittles(unit, quantity);\n \n             //Assert\n"}]},{"message":"The class name 'UnitsTests' should be 'UnitTests' to follow standard naming conventions for test classes.","fixFiles":[{"filePath":"src/MandMCounter.Tests/UnitsTests.cs","diff":"diff --git a/src/MandMCounter.Tests/UnitsTests.cs b/src/MandMCounter.Tests/UnitsTests.cs\n--- a/src/MandMCounter.Tests/UnitsTests.cs\n+++ b/src/MandMCounter.Tests/UnitsTests.cs\n@@ -7,7 +7,7 @@\n {\n     [System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]\n     [TestClass]\n-    public class UnitsTests\n+    public class UnitTests\n     {\n         [TestMethod]\n         public void GetUnitsForVolumeTest()\n"}]},{"message":"Use Assert.IsNotNull(results) instead of Assert.IsTrue(results != null) for clearer intent and better error messages.","fixFiles":[{"filePath":"src/MandMCounter.Tests/UnitsTests.cs","diff":"diff --git a/src/MandMCounter.Tests/UnitsTests.cs b/src/MandMCounter.Tests/UnitsTests.cs\n--- a/src/MandMCounter.Tests/UnitsTests.cs\n+++ b/src/MandMCounter.Tests/UnitsTests.cs\n@@ -18,7 +18,7 @@\n             List<string> results = Units.GetUnitsForVolume();\n \n             //Assert\n-            Assert.IsTrue(results != null);\n+            Assert.IsNotNull(results);\n             Assert.IsTrue(results.Count > 0);\n             Assert.IsTrue(string.IsNullOrEmpty(results[0]) == false);\n         }\n@@ -32,7 +32,7 @@\n             List<string> results = Units.GetUnitsForContainer();\n \n             //Assert\n-            Assert.IsTrue(results != null);\n+            Assert.IsNotNull(results);\n             Assert.IsTrue(results.Count > 0);\n             Assert.IsTrue(string.IsNullOrEmpty(results[0]) == false);\n         }\n"}]},{"message":"[nitpick] Consider using CollectionAssert.IsNotEmpty(results) or Assert.IsTrue(results.Any()) for more descriptive collection assertions.","fixFiles":[{"filePath":"src/MandMCounter.Tests/UnitsTests.cs","diff":"diff --git a/src/MandMCounter.Tests/UnitsTests.cs b/src/MandMCounter.Tests/UnitsTests.cs\n--- a/src/MandMCounter.Tests/UnitsTests.cs\n+++ b/src/MandMCounter.Tests/UnitsTests.cs\n@@ -19,7 +19,7 @@\n \n             //Assert\n             Assert.IsTrue(results != null);\n-            Assert.IsTrue(results.Count > 0);\n+            CollectionAssert.IsNotEmpty(results);\n             Assert.IsTrue(string.IsNullOrEmpty(results[0]) == false);\n       ...

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@samsmithnz samsmithnz marked this pull request as ready for review December 4, 2025 19:12
Co-authored-by: samsmithnz <8389039+samsmithnz@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 4, 2025

Pull Request Test Coverage Report for Build 19940932922

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-10.2%) to 71.815%

Files with Coverage Reduction New Missed Lines %
src/MandMCounter.Core/Calculator.cs 2 68.94%
Totals Coverage Status
Change from base Build 19940648005: -10.2%
Covered Lines: 96
Relevant Lines: 99

💛 - Coveralls

Copilot AI changed the title [WIP] Fix broad exception type in SkittleTests Improve test assertions and exception types for null unit validation Dec 4, 2025
Copilot AI requested a review from samsmithnz December 4, 2025 19:16
@samsmithnz samsmithnz enabled auto-merge December 4, 2025 19:42
@samsmithnz samsmithnz merged commit 0fece21 into main Dec 4, 2025
7 checks passed
@samsmithnz samsmithnz deleted the copilot/fix-exception-type-checks branch December 4, 2025 19:45
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.

2 participants