Skip to content

Conversation

pja-ant
Copy link
Contributor

@pja-ant pja-ant commented Sep 19, 2025

Motivation and Context

We've seen quite a few issues related to Unicode handling and while things appear to work, I noticed that we don't actually have tests for this (there's an example server though). Thought it would be useful to have a test to point people to if they expect Unicode handling isn't working and just ensure it doesn't regress.

How Has This Been Tested?

Run the new test

Breaking Changes

N/A

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

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

@pja-ant pja-ant requested review from a team and ochafik September 19, 2025 17:04
Copy link
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

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

RC for addressing nit

@felixweinberger felixweinberger added the needs more work Not ready to be merged yet, needs additional changes. label Sep 26, 2025
- Test Unicode text transmission in both directions (client→server→client)
- Verify Unicode handling in tool descriptions, arguments, and responses
- Verify Unicode handling in prompt descriptions and content
- Test with various Unicode scripts including Cyrillic, Chinese, Japanese,
  Korean, Arabic, Hebrew, Greek, emoji, and special characters
- Use multiprocessing to run server in separate process (avoids ResourceWarnings)
- Follow existing test patterns from test_streamable_http_manager.py
Consolidate multiprocessing, socket, and time imports at the top of the
file rather than importing them locally within fixtures. The imports
within run_unicode_server remain as they must be available in the
separate process context.
@pja-ant pja-ant force-pushed the tests/add-unicode-test branch from e178f6e to fad7e01 Compare September 29, 2025 12:34
Copy link
Contributor

@felixweinberger felixweinberger 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!

@felixweinberger felixweinberger merged commit 60f4b2d into main Sep 29, 2025
21 checks passed
@felixweinberger felixweinberger deleted the tests/add-unicode-test branch September 29, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more work Not ready to be merged yet, needs additional changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants