Skip to content
This repository was archived by the owner on Mar 24, 2025. It is now read-only.

Conversation

jtorreggiani
Copy link

Implements a basic test template that invokes all of the server methods end-to-end using an stdio client to the template.

Including a test in the template is useful both for end users and developers contributing to the project. When changes are made to core templates, the tests can be run on the test server to ensure things are running correctly.

Copy link

@darinkishore darinkishore left a comment

Choose a reason for hiding this comment

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

lgtm! Would be great to have this in the default implementation.

@darinkishore
Copy link

@jspahrsummers @dsp-ant

would be great to get this merged if it looks okay to yall !

@dsp-ant dsp-ant self-requested a review November 29, 2024 19:39
Copy link
Member

@dsp-ant dsp-ant 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 for your proposal.

I don't think we necessarily want to test the end-to-end flow. This would make a lot of assumptions about the environment that are easy enough to break.

Notable, the server is meant as a starting point. People will inherently edit it and instantly break the tests. I think for now I am more comfortable not including any tests.

("__init__.py.jinja2", "__init__.py", target_dir),
("server.py.jinja2", "server.py", target_dir),
("README.md.jinja2", "README.md", path),
("test_server.py.jinja2", "test_server.py", target_dir),
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the right directory tests should live. This would entangle them with the package.

async def test_client_server_interactions():
"""Fixture that starts the actual server and provides a connected session"""
server_params = StdioServerParameters(
command=f"{test_dir}/template_server/src/template_server/server.py",
Copy link
Member

Choose a reason for hiding this comment

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

This path assumes that the server is called template_server, which does not hold true..

@dsp-ant dsp-ant closed this Nov 29, 2024
@jtorreggiani
Copy link
Author

jtorreggiani commented Nov 30, 2024

Thank you for your proposal.

I don't think we necessarily want to test the end-to-end flow. This would make a lot of assumptions about the environment that are easy enough to break.

Notable, the server is meant as a starting point. People will inherently edit it and instantly break the tests. I think for now I am more comfortable not including any tests.

Thanks @dsp-ant for the consideration here. I think idea for these tests is to ensure that the starter server actually will run. I generated the template and it did not work out the of the box. There were implementation details in the template that were not documented. Without tests I think it will be easy for these template code to become out of date or break when changes are made without knowing.

Perhaps tests could be added to the repository itself and a they can be run against an instance of a test server generated with the tool?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants