TestServer: Server, but partial'd to run on a test socket
#565
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request introduces a Sequence diagram for TempServer creation and cleanupsequenceDiagram
participant Test Function
participant TempServer Fixture
participant Server
participant tmux Server Process
Test Function->>TempServer Fixture: Request TempServer
activate TempServer Fixture
TempServer Fixture->>Server: functools.partial(Server, on_init, socket_name_factory)
activate Server
Server->>tmux Server Process: Create tmux server with unique socket name
Server-->>TempServer Fixture: Server instance
deactivate Server
TempServer Fixture-->>Test Function: Server factory (partial)
deactivate TempServer Fixture
Test Function->>Server: Call Server()
activate Server
Server->>tmux Server Process: Create tmux server with unique socket name
Server-->>Test Function: Server instance
deactivate Server
loop For each created socket
Test Function->>TempServer Fixture: Test completes, finalizer called
activate TempServer Fixture
TempServer Fixture->>Server: Server(socket_name)
activate Server
Server->>tmux Server Process: Kill tmux server
deactivate Server
TempServer Fixture-->>Test Function: Cleanup complete
deactivate TempServer Fixture
end
Updated class diagram for the Server classclassDiagram
class Server {
-socket_name: str
-socket_path: str
-config_file: str
-colors: str
-on_init: callable
-socket_name_factory: callable
+__init__(
socket_name: str | None,
socket_path: str | pathlib.Path | None,
config_file: str | None,
colors: int | None,
on_init: t.Callable[[Server], None] | None,
socket_name_factory: t.Callable[[], str] | None
)
+is_alive() bool
}
note for Server "Added on_init and socket_name_factory attributes and parameters to the constructor"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #565 +/- ##
==========================================
+ Coverage 88.67% 88.72% +0.04%
==========================================
Files 36 36
Lines 3921 4027 +106
Branches 362 372 +10
==========================================
+ Hits 3477 3573 +96
- Misses 307 312 +5
- Partials 137 142 +5 ☔ View full report in Codecov by Sentry. |
a7aaa3e to
f4bf7fb
Compare
|
@sourcery-ai review |
TestServer: Server, but partial'd to run on a test socketTempServer: Server, but partial'd to run on a test socket
There was a problem hiding this comment.
Hey @tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- The
TempServerfixture is a great addition for testing, ensuring isolated tmux environments. - Consider adding a brief explanation of why
time.sleep(0.1)is necessary intest_temp_server_cleanup.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- The tests for
TempServeruseassert server.is_alive() is Falsewhich seems redundant -assert not server.is_alive()is more concise. - Consider adding a helper function to encapsulate the server cleanup logic to avoid repetition in the tests.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1c41120 to
45873b9
Compare
Add two new optional parameters to Server constructor:
- socket_name_factory: Callable[[], str]
Generates unique socket names for new servers. Used when socket_name
is not provided. Useful for creating multiple servers with unique names.
- on_init: Callable[[Server], None]
Callback that runs after server initialization. Useful for tracking
server instances and performing cleanup in tests.
The socket_name_factory is tried after socket_name, maintaining backward
compatibility while adding flexibility for dynamic socket name generation.
Example:
def socket_name_factory() -> str:
return f"tmux_{next(counter)}"
server = Server(socket_name_factory=socket_name_factory)
This enables better testing patterns and more flexible server creation,
particularly in test environments where unique socket names are needed.
There was a problem hiding this comment.
Hey @tony - I've reviewed your changes - here's some feedback:
Overall Comments:
- The addition of
on_initandsocket_name_factoryto theServerclass seems like a useful extension. - The
TempServerfixture looks like a great way to improve testing, especially for docstrings.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Adds a new pytest fixture TestServer that returns a factory for creating
tmux servers with unique socket names. Each server is automatically cleaned
up when the test completes.
The fixture provides:
- Factory function returning partial'd Server instances
- Unique socket names for each server instance
- Automatic cleanup through pytest's addfinalizer
- Support for custom tmux configs in tests
Example usage:
def test_example(TestServer):
Server = TestServer() # Get partial'd Server
server = Server() # Create server instance
server.new_session()
TempServer: Server, but partial'd to run on a test socketTestServer: Server, but partial'd to run on a test socket
|
@sourcery-ai review |
|
@sourcery-ai dismiss |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
It's tricky to
doctesttheServerobject inserver.pydocstrings sinceServerruns outside the sockets we want.Make a
TestServerthat can be served todoctest_namespaceasServerSummary by Sourcery
Introduce the
TempServerfixture to facilitate testing with multiple tmux servers. Addon_initandsocket_name_factorycallbacks to theServerclass. Update documentation and tests accordingly.Enhancements:
on_initandsocket_name_factoryparameters to theServerclass to allow for custom initialization and socket name generation.TempServerfixture to create temporary, uniquely named tmux servers for testing, automatically cleaning up after each test.TempServerfixture, covering server creation, cleanup, configuration loading, and multiple server coexistence.doctest_namespaceto useTempServerto provide a partial'dServerobject, enabling doctesting of theServerclass within docstringsDocumentation:
TempServerfixture and its usage with and without custom configuration files.Serverfixture fromTempServerfor use in docstrings