Skip to content

Conversation

@domdomegg
Copy link
Member

Description

This PR adds comprehensive test coverage for the get_local_tz function in the time server, ensuring proper handling of timezone detection across different platforms.

Server Details

  • Server: Time
  • Changes to: tests

Motivation and Context

While reviewing PR #1272 (which addresses bug #489 about Windows timezone issues), I noticed that the main branch already has a robust fix using the tzlocal library. However, there were no tests covering the get_local_tz function to ensure it properly handles various edge cases.

This PR adds tests to verify:

  • Timezone override functionality
  • Invalid timezone handling
  • Proper integration with tzlocal.get_localzone_name()
  • Error handling when timezone cannot be determined
  • Support for various IANA timezone names

How Has This Been Tested?

  • All 38 tests pass (including 13 new tests for get_local_tz)
  • Tests use mocking to simulate different timezone scenarios
  • Tests follow the existing test file conventions (standalone functions, not classes)
  • Code passes ruff linting

Breaking Changes

No

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Test improvement

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

Additional context

This PR complements the existing solution in main that uses tzlocal for robust timezone detection. The tests ensure that the current implementation correctly handles edge cases that were identified in issue #489 (Windows "Pacific Standard Time" timezone) and related issues #612 and #786.

The main branch's solution using tzlocal.get_localzone_name() is superior to the approach in PR #1272 as it's a well-maintained library specifically designed for cross-platform timezone detection.

@domdomegg domdomegg enabled auto-merge August 1, 2025 18:10
@olaservo olaservo added server-time Reference implementation for the Time MCP server - src/time enhancement New feature or request labels Aug 2, 2025
Copy link
Member

@olaservo olaservo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@domdomegg domdomegg merged commit a863125 into main Aug 2, 2025
22 checks passed
@domdomegg domdomegg deleted the adamj/time-tests branch August 2, 2025 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request server-time Reference implementation for the Time MCP server - src/time

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants