-
Notifications
You must be signed in to change notification settings - Fork 8k
Compare DateTime instances respecting their Kind in Test-Item (#25013) #26250
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?
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 fixes a bug in the Test-Path cmdlet where DateTime comparisons with -OlderThan and -NewerThan parameters were not correctly respecting the DateTime Kind property (Local, UTC, or Unspecified). The fix ensures that all DateTime values are properly converted to UTC before comparison.
Key Changes:
- Updated the comparison logic to convert DateTime instances to UTC while respecting their Kind property
- Modified test setup to use UTC-based DateTime values and file timestamps
- Added timezone manipulation in tests using reflection to ensure cross-platform test reliability
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| FileSystemProvider.cs | Fixed DateTime comparison logic to convert to UTC while handling Unspecified DateTimeKind |
| Test-Path.Tests.ps1 | Updated tests to use UTC DateTime values and added timezone manipulation for reliable cross-platform testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| DateTime olderThanUtc = olderThan.Kind == DateTimeKind.Unspecified | ||
| ? DateTime.SpecifyKind(olderThan, DateTimeKind.Utc) | ||
| : olderThan.ToUniversalTime(); |
Copilot
AI
Oct 20, 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.
When DateTimeKind is Unspecified, using DateTime.SpecifyKind to treat it as UTC could lead to incorrect comparisons. An Unspecified DateTime should typically be assumed to be local time and converted using DateTime.ToUniversalTime(), or the behavior should be explicitly documented. The current implementation may produce unexpected results when users pass DateTime values without specifying a Kind.
Copilot uses AI. Check for mistakes.
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.
I agree with Copilot:
| DateTime olderThanUtc = olderThan.Kind == DateTimeKind.Unspecified | |
| ? DateTime.SpecifyKind(olderThan, DateTimeKind.Utc) | |
| : olderThan.ToUniversalTime(); | |
| DateTime olderThanUtc = olderThan.ToUniversalTime(); |
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.
Initially I thought that interpreting DateTimeKind.Unspecified as UTC would be more transparent; however, also in order to stick to the previous behavior, now I agree with the suggestion.
I will apply the same also below.
| DateTime newerThanUtc = newerThan.Kind == DateTimeKind.Unspecified | ||
| ? DateTime.SpecifyKind(newerThan, DateTimeKind.Utc) | ||
| : newerThan.ToUniversalTime(); |
Copilot
AI
Oct 20, 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.
When DateTimeKind is Unspecified, using DateTime.SpecifyKind to treat it as UTC could lead to incorrect comparisons. An Unspecified DateTime should typically be assumed to be local time and converted using DateTime.ToUniversalTime(), or the behavior should be explicitly documented. The current implementation may produce unexpected results when users pass DateTime values without specifying a Kind.
Copilot uses AI. Check for mistakes.
|
@microsoft-github-policy-service agree |
PR Summary
Fix #25013.
When comparing
DateTimeinstances provided as parameter-OlderThan/-NewerThanin cmdletTest-Item, now theKind(local, UTC or unspecified) is correctly taken into account.Tests have also been updated, but with a caveat!⚠️
The impacted tests acquire significance when run in a timezone with some test-dependent criteria. To ensure the test behavior behavior in a cross-platform, revertable, scoped (in-process, non-system-wide) fashion, I temporarily set a custom timezone for the duration of each test. Unfortunately, the in-process local timezone can be set only via reflection, so any improvements to this tweaky approach are welcome.
PR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header