Improved Unit Test Coverage for Project #72
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds comprehensive unit tests: new test suites for DirectoryProvider, FileProvider, and UrlSlugGenerator; updates TimeProviderTests with parsing, epoch/timezone tests and timing assertion adjustments. All changes are test-only; no production code modified. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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: 1
🧹 Nitpick comments (4)
src/NetCore.Utilities.Tests/TimeProviderTests.cs (1)
149-156: Consider makingUtcNowSecondsSinceEpochtest tolerant instead of strictBoth
SecondsSinceEpoch(_timeProvider.UtcNow)and_timeProvider.UtcNowSecondsSinceEpoch()depend on wall‑clock time; under heavy load they could drift by a second and sporadically fail. Allowing a ±1 second window would keep the intent while avoiding rare flakes.Example adjustment:
- public void UtcNowSecondsSinceEpoch_ShouldMatchSecondsSinceEpochOfUtcNow() - { - var utcNow = _timeProvider.UtcNow; - var expected = _timeProvider.SecondsSinceEpoch(utcNow); - var actual = _timeProvider.UtcNowSecondsSinceEpoch(); - Assert.Equal(expected, actual); - } + public void UtcNowSecondsSinceEpoch_ShouldBeWithinOneSecondOfSecondsSinceEpochOfUtcNow() + { + var expected = _timeProvider.SecondsSinceEpoch(_timeProvider.UtcNow); + var actual = _timeProvider.UtcNowSecondsSinceEpoch(); + + var delta = Math.Abs((long)expected - (long)actual); + Assert.InRange(delta, 0, 1); + }src/NetCore.Utilities.Tests/FileProviderTests.cs (1)
239-263: Optional: strengthenReplacetests by asserting backup contentsRight now the
Replacetests ensure that the destination contains the source content and that the backup file exists. To fully pin wrapper behavior toFile.Replace, you might also assert that the backup contains the original destination content.For example, in
Replace_ShouldReplaceFileContents:- _fileProvider.Replace(src, dest, backup); - Assert.Equal("Source", _fileProvider.ReadAllText(dest)); - Assert.True(_fileProvider.Exists(backup)); + _fileProvider.Replace(src, dest, backup); + Assert.Equal("Source", _fileProvider.ReadAllText(dest)); + Assert.True(_fileProvider.Exists(backup)); + Assert.Equal("Destination", _fileProvider.ReadAllText(backup));Same idea could be applied to the
Replace_WithIgnoreMetadataErrorsvariant.src/NetCore.Utilities.Tests/DirectoryProviderTests.cs (2)
188-219: Optional: timestamp tests could better isolate which property is being set
SetAndGetLastAccessTime_ShouldWork,SetAndGetLastWriteTime_ShouldWork, andSetAndGetCreationTime_ShouldWorkcurrently assert only that the year of the retrieved timestamp matches the year you wrote. That’s fine as a smoke test, but similar to theFileProvidercase, it won’t catch a future regression where setters are accidentally wired to the wrong underlyingDirectoryAPIs.If you want stricter coverage, consider patterns where you:
- Baseline one timestamp to year A and another to year B.
- Invoke a single setter.
- Assert that only the intended timestamp’s year changes.
That would make these tests robust against mis‑wiring while still tolerating filesystem granularity.
221-232:GetLogicalDrives_ShouldReturnDrivesmay be brittle across platformsThe test currently asserts that:
GetLogicalDrives()is non‑empty.- Its length exactly matches
DriveInfo.GetDrives().Length.- Every
DriveInfo.Nameis contained in the returned array.On Windows this likely holds if
DirectoryProvider.GetLogicalDrivesdelegates toDirectory.GetLogicalDrives, but on Linux/macOS the relationship betweenDriveInfo.GetDrives()and any logical‑drive list can differ, which could make this test fail even when the wrapper is correctly forwarding.You might want to relax the assertion to something like “every drive returned by the provider exists in the
DriveInfoset” or simply assert non‑emptiness and that returned names are a subset ofDriveInfonames.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/NetCore.Utilities.Tests/DirectoryProviderTests.cs(1 hunks)src/NetCore.Utilities.Tests/FileProviderTests.cs(1 hunks)src/NetCore.Utilities.Tests/TimeProviderTests.cs(3 hunks)src/NetCore.Utilities.Tests/UrlSlugGeneratorTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/NetCore.Utilities.Tests/FileProviderTests.cs (1)
src/NetCore.Utilities/FileProvider.cs (1)
FileProvider(401-717)
src/NetCore.Utilities.Tests/TimeProviderTests.cs (1)
src/NetCore.Utilities/TimeProvider.cs (12)
TryParse(85-85)TryParse(105-105)TryParse(208-208)TryParse(211-211)TryParseExact(123-124)TryParseExact(142-143)TryParseExact(214-215)TryParseExact(218-219)SecondsSinceEpoch(152-152)SecondsSinceEpoch(222-227)UtcNowSecondsSinceEpoch(160-160)UtcNowSecondsSinceEpoch(230-233)
src/NetCore.Utilities.Tests/UrlSlugGeneratorTests.cs (1)
src/NetCore.Utilities/UrlSlugGenerator.cs (1)
UrlSlugGenerator(22-39)
⏰ 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). (1)
- GitHub Check: Analyze Code Quality
🔇 Additional comments (11)
src/NetCore.Utilities.Tests/UrlSlugGeneratorTests.cs (1)
6-62: UrlSlugGenerator tests cover key behaviors and align with implementationThe test suite nicely exercises normal, punctuation-heavy, edge, and invalid inputs, plus the dash-collapsing and trimming behavior. The expectations match the current
UrlSlugGeneratorimplementation and should give good regression coverage with minimal brittleness.src/NetCore.Utilities.Tests/TimeProviderTests.cs (4)
19-22: Using a 1‑second tolerance for Now/UtcNow is a good, low‑flakiness patternComparing
TotalSecondswithin a small tolerance instead of asserting exact equality gives the right balance between verifying the wrapper and avoiding clock‑scheduling flakes.Also applies to: 36-40
62-90: Parse / TryParse / TryParseExact tests nicely lock wrapper behavior toDateTimeThese tests consistently use
DateTime.Parse/DateTime.TryParse/DateTime.TryParseExactas oracles (with/without provider and styles), which is exactly what you want to ensure the wrapper stays a thin delegation layer over the BCL behavior.Also applies to: 92-112, 114-138
140-147: SecondsSinceEpoch constant matches the underlying implementationThe hard‑coded value
1546345815for2019-01-01 12:30:15Zlines up with theSecondsSinceEpochimplementation (epoch at 1970‑01‑01T00:00:00Z and returningTotalSecondsasulong), so this is a good regression guard for that helper.
158-192: Timezone conversion tests cover both success and failure paths wellThe new
ConvertTimeFromUtc/ConvertTimeToUtctests validate both unknown‑timezone failure (TimeZoneNotFoundException) and correct mapping throughTimeZoneInfo.Local, which should catch most wrapper mistakes around timezone ID usage and direction of conversion.src/NetCore.Utilities.Tests/FileProviderTests.cs (3)
17-39: Per‑test temp directory setup/teardown is clean and test‑friendlyCreating a unique temp root per test and cleaning it up in
Disposeis a solid pattern that keeps the filesystem tests isolated and avoids cross‑test interference.
41-215: Broad file IO and stream coverage is well‑structuredThe tests around
WriteAllText/ReadAllText, line‑based IO, append operations, bytes round‑trip, and the variousOpen*/Create*overloads do a good job of pinning theFileProvidertoSystem.IO.Filesemantics, including encoding‑specific paths and stream capabilities.Also applies to: 239-263, 341-395
341-395: ReadLines and Create/Open overload tests nicely cover enumeration and stream behaviorThe
ReadLines(with and without encoding) plusCreate/Openoverload tests give good confidence that the provider’s enumerable and stream APIs behave as expected (readability/writability, correct data).src/NetCore.Utilities.Tests/DirectoryProviderTests.cs (3)
16-32: Temp root management viaIDisposablekeeps directory tests isolatedCreating a per‑test
_testRootunder the system temp path and cleaning it up inDisposeis a good pattern for avoiding cross‑test interference and leftover state on the filesystem.
34-186: Directory creation/deletion, enumeration, and move tests give good coverageThe tests around
CreateDirectory,Delete(including recursive), directory/file enumerations (with/without search patterns), existence checks, andMoveall map directly to the underlyingSystem.IO.Directory/Filebehavior and should quickly surface wrapper regressions.
234-241: Parent directory test correctly tiesGetParentto the temp root
GetParent_ShouldReturnParentDirectoryInfois a nice sanity check thatGetParentreturns_testRootfor a child under_testRoot, which should quickly catch mapping mistakes in theDirectoryProvider.GetParentimplementation.
| [Fact] | ||
| public void GetAndSetAttributes_ShouldWork() | ||
| { | ||
| var path = GetTestFilePath(); | ||
| _fileProvider.WriteAllText(path, "Attributes"); | ||
| _fileProvider.SetAttributes(path, FileAttributes.ReadOnly); | ||
| var attrs = _fileProvider.GetAttributes(path); | ||
| Assert.True(attrs.HasFlag(FileAttributes.ReadOnly)); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetAndSetCreationTime_ShouldWork() | ||
| { | ||
| var path = GetTestFilePath(); | ||
| _fileProvider.WriteAllText(path, "CreationTime"); | ||
| var now = DateTime.Now; | ||
| _fileProvider.SetCreationTime(path, now); | ||
| var actual = _fileProvider.GetCreationTime(path); | ||
| Assert.Equal(now.Year, actual.Year); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetAndSetCreationTimeUtc_ShouldWork() | ||
| { | ||
| var path = GetTestFilePath(); | ||
| _fileProvider.WriteAllText(path, "CreationTimeUtc"); | ||
| var utcNow = DateTime.UtcNow; | ||
| _fileProvider.SetCreationTimeUtc(path, utcNow); | ||
| var actual = _fileProvider.GetCreationTimeUtc(path); | ||
| Assert.Equal(utcNow.Year, actual.Year); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetAndSetLastAccessTime_ShouldWork() | ||
| { | ||
| var path = GetTestFilePath(); | ||
| _fileProvider.WriteAllText(path, "LastAccessTime"); | ||
| var now = DateTime.Now; | ||
| _fileProvider.SetLastAccessTime(path, now); | ||
| var actual = _fileProvider.GetLastAccessTime(path); | ||
| Assert.Equal(now.Year, actual.Year); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetAndSetLastAccessTimeUtc_ShouldWork() | ||
| { | ||
| var path = GetTestFilePath(); | ||
| _fileProvider.WriteAllText(path, "LastAccessTimeUtc"); | ||
| var utcNow = DateTime.UtcNow; | ||
| _fileProvider.SetLastAccessTimeUtc(path, utcNow); | ||
| var actual = _fileProvider.GetLastAccessTimeUtc(path); | ||
| Assert.Equal(utcNow.Year, actual.Year); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetAndSetLastWriteTime_ShouldWork() | ||
| { | ||
| var path = GetTestFilePath(); | ||
| _fileProvider.WriteAllText(path, "LastWriteTime"); | ||
| var now = DateTime.Now; | ||
| _fileProvider.SetLastWriteTime(path, now); | ||
| var actual = _fileProvider.GetLastWriteTime(path); | ||
| Assert.Equal(now.Year, actual.Year); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void GetAndSetLastWriteTimeUtc_ShouldWork() | ||
| { | ||
| var path = GetTestFilePath(); | ||
| _fileProvider.WriteAllText(path, "LastWriteTimeUtc"); | ||
| var utcNow = DateTime.UtcNow; | ||
| _fileProvider.SetLastWriteTimeUtc(path, utcNow); | ||
| var actual = _fileProvider.GetLastWriteTimeUtc(path); | ||
| Assert.Equal(utcNow.Year, actual.Year); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the FileProvider.cs file
find . -name "FileProvider.cs" -type fRepository: IowaComputerGurus/netcore.utilities
Length of output: 118
🏁 Script executed:
# List the overall structure to understand the repository
git ls-files | head -20Repository: IowaComputerGurus/netcore.utilities
Length of output: 758
🏁 Script executed:
# Read the FileProvider.cs file to check the implementations
cat -n ./src/NetCore.Utilities/FileProvider.csRepository: IowaComputerGurus/netcore.utilities
Length of output: 37975
🏁 Script executed:
# Read the test file to verify test structure
sed -n '265,339p' ./src/NetCore.Utilities.Tests/FileProviderTests.csRepository: IowaComputerGurus/netcore.utilities
Length of output: 3106
🏁 Script executed:
# Read more lines to see all the tests mentioned
sed -n '296,350p' ./src/NetCore.Utilities.Tests/FileProviderTests.csRepository: IowaComputerGurus/netcore.utilities
Length of output: 2126
🏁 Script executed:
# Get the remaining test
sed -n '340,360p' ./src/NetCore.Utilities.Tests/FileProviderTests.csRepository: IowaComputerGurus/netcore.utilities
Length of output: 892
SetLastAccessTimeUtc and SetLastWriteTime have mapping bugs that current tests fail to catch
The FileProvider implementation contains two bugs that the current tests are too weak to detect:
- Line 660:
SetLastAccessTimeUtccallsFile.SetCreationTimeUtcinstead ofFile.SetLastAccessTimeUtc - Line 666:
SetLastWriteTimecallsFile.SetLastAccessTimeinstead ofFile.SetLastWriteTime
The current tests only assert now.Year == actual.Year, which pass despite the wrong methods being called because the filesystem timestamps often coincide in year values by chance. To catch these mis-mappings, tests should:
- Set different timestamps to clearly different years (e.g., creation time to 2000, then access/write time to 2010)
- Assert that the intended timestamp changes to the new value
- Assert that unrelated timestamps remain at their baseline value
For example, GetAndSetLastWriteTime_ShouldWork should set access time to an earlier year, then write time to a later year, and verify both values independently. This would cause tests to fail if SetLastWriteTime mistakenly calls SetLastAccessTime.
🤖 Prompt for AI Agents
In src/NetCore.Utilities.Tests/FileProviderTests.cs around lines 265-339, the
tests are too weak to catch mapping bugs (SetLastAccessTimeUtc calls
SetCreationTimeUtc and SetLastWriteTime calls SetLastAccessTime); update the
tests to set clearly different timestamps (e.g., creation=2000,
access/write=2010), then set the target timestamp and assert the target changed
to the expected year and that the unrelated timestamps remain at their baseline
values so wrong method calls will fail; also fix the FileProvider implementation
mappings (ensure SetLastAccessTimeUtc calls File.SetLastAccessTimeUtc and
SetLastWriteTime calls File.SetLastWriteTime) if you prefer to correct code
rather than only strengthen tests.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.