-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Add path utils and test config for filesystem #1405
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
…age, remove problematic test files - Fixed UNC path handling bug in normalizePath function to preserve leading double backslashes - Added comprehensive test coverage for drive letter capitalization and UNC paths - Removed file-operations.test.ts and core-functionality.test.ts as they were testing their own code rather than actual server functionality - All path-utils tests now pass with 100% coverage of the actual utility functions
Thanks for checking this out, I originally created the PR to add tests for another PR that someone else had opened related to better Windows handling: #543 I think things got weird with the state of the PR and I obviously didn't look closely enough at what part of this was testing, I appreciate that you did. I think its all good now? |
Those changes were a bit different from the suggested ones that passed locally. I'm getting these errors now:
Analysis of the current failures:
I'll add the suggestions to |
- Added test job that runs before build - Tests are conditionally executed only for packages that have test scripts - Filesystem tests will now run automatically in CI on every push and PR - Build job now depends on successful test completion
Added checks for Typescript servers to run tests if found, here is an example run: https://github.com/modelcontextprotocol/servers/actions/runs/15780741769/job/44485530850?pr=1405 |
Co-authored-by: Cliff Hall <cliff@futurescale.com>
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.
Approving to get a big win in for the day, but would be good to get coverage on line 76 of the path-utils.ts at some point. This is a massive improvement in confidence for the filesystem server.
Add path utils and test config for filesystem
Description
Separating out the test config and utils from this older PR to simplify the actual changes needed to the server: #543
Plan is to merge this, then the original PR author can update their branch to reference the utils (or create a new one with just those changes).
Server Details
Motivation and Context
Original PR includes a large number of changes so the intention is to split it up so we can finally integrate the Windows path fixes into the server.
How Has This Been Tested?
Breaking Changes
None
Types of changes
Checklist