-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Improve NoBackwardTimeZones test diagnostics for duplicate timezone display names #121033
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
Conversation
…ttribute Co-authored-by: stephentoub <[email protected]>
|
Tagging subscribers to this area: @dotnet/area-system-datetime |
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 addresses a test failure on macOS 26 Preview 3 where the NoBackwardTimeZones test fails due to duplicate timezone display names. The test expects all system timezones to have unique display names, but macOS 26 Preview 3 has 419 timezones with only 418 unique display names.
- Added
[ActiveIssue]attribute to skip the failing test on macOS platforms - References the existing issue #64111 that documents this behavior on Linux
| [ConditionalFact] | ||
| [ActiveIssue("https://github.com/dotnet/runtime/issues/64111", TestPlatforms.Linux)] | ||
| [ActiveIssue("https://github.com/dotnet/runtime/issues/117731", TestPlatforms.Android)] | ||
| [ActiveIssue("https://github.com/dotnet/runtime/issues/64111", TestPlatforms.OSX)] |
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.
Is this still a problem on macOS 26 as reported in the bug?
This test is not failing on macOS 26 outer loop extra platform runs: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1183077&view=ms.vss-test-web.build-test-results-tab&runId=32734848&paneView=debug
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.
Not sure. I'm unable to check locally. The issue was opened recently, though.
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.
cc: @vcsjones
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.
Still fails for me locally on macOS 26.0.1. The duplicate timezones are ["(UTC-05:00) Eastern Time (New York)", "(UTC) Coordinated Universal Time"].
It's failing differently from me in the original issue I reported, too.
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.
You're right - I've removed the OSX ActiveIssue attribute. The improved test now provides better diagnostics if the issue resurfaces. Commit: 52fa883
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 think [ActiveIssue("https://github.com/dotnet/runtime/issues/64111", TestPlatforms.Linux)] is skipping the MacOS run. I'll try to run locally to see if this is the case.
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.
No, ActivityIssue isn’t related to this. It seems there’s an underlying issue in the product that needs further investigation. When running the full TZ test suite, the test passes, but when running only the NoBackwardTimeZones method, it fails. This likely means other tests are updating the cache in a way that affects GetSystemTimeZones, causing the test to pass when run together. However, when NoBackwardTimeZones runs alone, it creates the cache for the first time, which exposes the issue. I’ll look into this further when I have some time, but for now, we should assume the test can fail on macOS 26.
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.
Should both Linux and macOS be disable against the same issue - is it likely that Linux and macOS failures have the same root cause?
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.
Most likely yes, the TZ handling on MacOS is the same as Linux.
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.
| [ActiveIssue("https://github.com/dotnet/runtime/issues/64111", TestPlatforms.OSX)] | ||
| public static void NoBackwardTimeZones() | ||
| { | ||
| if (OperatingSystem.IsAndroid() && !OperatingSystem.IsAndroidVersionAtLeast(26)) |
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.
@copilot Could you please update the test to show the duplicate timezones so that we get more details about the failure next time?
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.
Could you please update the test to show the duplicate timezones so that we get more details about the failure next time?
@jkotas, and presumably remove the ActiveIssue?
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.
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.
Updated the test to show duplicate timezones. When duplicates are found, the test now outputs: "Found N duplicate display name(s): 'DisplayName' -> [TimeZoneId1, TimeZoneId2]" for each duplicate. Commit: 52fa883
Co-authored-by: jkotas <[email protected]>
|
Closing this in the favor of #121095 |
Improves the
System.Tests.TimeZoneInfoTests.NoBackwardTimeZonestest to provide better diagnostics when duplicate timezone display names are detected.Problem
The test was reported as failing on macOS 26 Preview 3 with:
The test continues to fail on macOS 26.0.1 locally with duplicate timezone display names for "(UTC-05:00) Eastern Time (New York)" and "(UTC) Coordinated Universal Time".
Solution
Enhanced the test to provide detailed diagnostic information when duplicate display names are found:
HashSet<String>toDictionary<String, List<String>>to track which timezone IDs map to each display name"Found 2 duplicate display name(s): '(UTC-05:00) Eastern Time (New York)' -> [America/New_York, US/Eastern], '(UTC) Coordinated Universal Time' -> [UTC, Etc/UTC]"This provides much better diagnostics for debugging timezone data issues across different platforms while maintaining the test's original purpose of verifying unique display names. The test will now fail on macOS with helpful diagnostic information instead of being skipped.
Fixes #117422
Original prompt
Fixes #117422
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.