CLI 2024: Re-add command line interface #549
Conversation
|
Warning Rate limit exceeded@amotl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
WalkthroughThe changes in this pull request involve updates to the Responder framework's package configuration, installation instructions, and documentation. Key modifications include the addition of a console script entry point in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
setup.py
Outdated
| @@ -20,7 +20,9 @@ | |||
| required = [ | |||
| "aiofiles", | |||
| "apispec>=1.0.0b1", | |||
| "asgiref", | |||
There was a problem hiding this comment.
What's asgiref for in this context? Can it be removed?
There was a problem hiding this comment.
Does anyone know why this would be needed?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
amotl
force-pushed
the
bring-back-cli
branch
3 times, most recently
from
2b58380 to
5be3422
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
amotl
force-pushed
the
bring-back-cli
branch
2 times, most recently
from
6b69a75 to
703720b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
amotl
force-pushed
the
bring-back-cli
branch
2 times, most recently
from
f724117 to
e61876a
Compare
amotl
changed the title
amotl
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
amotl
added
feature
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
amotl
force-pushed
the
bring-back-cli
branch
2 times, most recently
from
d4b7544 to
d6133ce
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
tests/util.py (2)
59-101: Consider adding debug logging for error cases.While the error handling is robust, adding debug logs for specific error cases would aid in troubleshooting.
Consider this enhancement:
if error_number in transient_socket_error_numbers(): + logger.debug(f"Ignoring transient error {error_number} while connecting to {endpoint}") pass # Unexpected error. else: + logger.debug(f"Encountered unexpected error {error_number} while connecting to {endpoint}") raise RuntimeError( f"Unexpected error while connecting to {endpoint}: {error_number}" )
103-135: Consider adding debug logging for consistency with wait_server_tcp.For consistency with
wait_server_tcp, consider adding similar debug logging.Consider this enhancement:
url = f"{protocol}://{host}:{port}/" + logger.debug(f"Waiting for endpoint: {url}") for attempt in range(1, attempts + 1): try: requests.get(url, timeout=delay / 2) # Shorter timeout for connection break except requests.exceptions.RequestException: + logger.debug(f"Connection attempt {attempt}/{attempts} failed for {url}") if attempt < attempts: # Don't sleep on last attempt time.sleep(delay)responder/ext/cli.py (1)
33-46: Consider reorganizing imports for better readability.The imports could be better organized following the common pattern: standard library, third-party packages, and local imports, each group separated by a blank line.
-import logging -import platform -import subprocess -import sys -import typing as t -from pathlib import Path - -import docopt - -from responder.__version__ import __version__ -from responder.util.python import InvalidTarget, load_target +import logging +import platform +import subprocess +import sys +import typing as t +from pathlib import Path + +import docopt + +from responder.__version__ import __version__ +from responder.util.python import InvalidTarget, load_targetresponder/util/cmd.py (1)
170-198: Consider closing socket explicitly in wait_until_ready.The socket connection test uses a context manager, but it's good practice to explicitly close the socket to ensure immediate resource release, especially in a loop.
- try: - with socket.create_connection( - ("localhost", self.port), timeout=request_timeout - ): - return True + sock = None + try: + sock = socket.create_connection( + ("localhost", self.port), timeout=request_timeout + ) + return True except ( socket.timeout, ConnectionRefusedError, socket.gaierror, OSError, ) as ex: logger.debug(f"Server not ready yet: {ex}") time.sleep(delay) + finally: + if sock: + sock.close()
🛑 Comments failed to post (3)
tests/util.py (1)
21-34: 🛠️ Refactor suggestion
Consider enhancing error handling for socket operations.
While the implementation is solid, it could benefit from more specific error handling.
Consider this enhancement:
def random_port() -> int: sock = socket.socket() try: sock.bind(("", 0)) return sock.getsockname()[1] + except socket.error as e: + raise RuntimeError(f"Failed to allocate random port: {e}") from e finally: sock.close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def random_port() -> int: """ Return a random available port by binding to port 0. Returns: int: An available port number that can be used for testing. """ sock = socket.socket() try: sock.bind(("", 0)) return sock.getsockname()[1] except socket.error as e: raise RuntimeError(f"Failed to allocate random port: {e}") from e finally: sock.close()responder/ext/cli.py (1)
106-111: 🛠️ Refactor suggestion
Consider using a custom exception for CLI errors.
Instead of raising a generic
ValueError, consider creating a custom exception likeCLIErrorthat better represents the domain-specific error condition. This would make it easier to handle CLI-specific errors differently from other value errors.+class CLIError(Exception): + """Raised when CLI encounters an error condition.""" + pass - raise ValueError( + raise CLIError( f"{ex}. " "Use either a Python module entrypoint specification, " "a filesystem path, or a remote URL. " "See also https://responder.kennethreitz.org/cli.html." ) from exCommittable suggestion was skipped due to low confidence.
responder/util/cmd.py (1)
25-40: 🛠️ Refactor suggestion
Consider caching the program path.
The
path()method searches PATH on every call. Consider caching the result to improve performance.class ResponderProgram: + _cached_path = None + @staticmethod def path(): + if ResponderProgram._cached_path is not None: + return ResponderProgram._cached_path + name = "responder" if sys.platform == "win32": name = "responder.exe" program = shutil.which(name) if program is None: paths = os.environ.get("PATH", "").split(os.pathsep) raise RuntimeError( f"Could not find '{name}' executable in PATH. " f"Please ensure Responder is installed correctly. " f"Searched in: {', '.join(paths)}" ) logger.debug(f"Found responder program: {program}") + ResponderProgram._cached_path = program return programCommittable suggestion was skipped due to low confidence.
| Commands: | ||
| run Start the application server | ||
| build Build frontend assets using npm |
There was a problem hiding this comment.
@kennethreitz: Now after learning it, I think the second subcommand responder build is debatable. Did you keep and/or evolve it within Dyne, @tabotkevin?
The `debug` argument no longer exists. Let's adjust the `log_level` to `debug` instead.
Install: pip install 'responder[cli]' The CLI is an optional subsystem from now on.
Also, refactor to `responder.ext.cli`.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
setup.py (1)
Line range hint
122-141: Consider line length compliance.The test requirements line exceeds the 90-character limit.
- "test": ["flask", "mypy", "pytest", "pytest-cov", "pytest-mock", "pytest-rerunfailures"], + "test": [ + "flask", + "mypy", + "pytest", + "pytest-cov", + "pytest-mock", + "pytest-rerunfailures", + ],🧰 Tools
🪛 Ruff (0.8.2)
141-141: Line too long (97 > 90)
(E501)
responder/util/cmd.py (4)
26-41: Consider caching the responder path.The
path()method searches PATH on every call. Consider caching the result to improve performance.class ResponderProgram: + _cached_path = None + @staticmethod def path(): + if ResponderProgram._cached_path is not None: + return ResponderProgram._cached_path + name = "responder" if sys.platform == "win32": name = "responder.exe" program = shutil.which(name) if program is None: paths = os.environ.get("PATH", "").split(os.pathsep) raise RuntimeError( f"Could not find '{name}' executable in PATH. " f"Please install Responder with 'pip install --upgrade responder[cli]'. " f"Searched in: {', '.join(paths)}" ) logger.debug(f"Found responder program: {program}") + ResponderProgram._cached_path = program return program
137-142: Add process synchronization.The process management between
run()andstop()methods could have race conditions. Consider adding a lock:def __init__(self, target: str, port: int = 5042, limit_max_requests: int = None): # ... existing code ... self.daemon = True + self._process_lock = threading.Lock() def run(self): # ... command setup ... + with self._process_lock: self.process = subprocess.Popen( command, env=env, universal_newlines=True, ) self.process.wait() def stop(self): + with self._process_lock: if self.process and self.process.poll() is None: self.process.terminate()
171-204: Improve server readiness detection error handling.The
wait_until_readymethod could benefit from more detailed error reporting:def wait_until_ready(self, timeout=30, request_timeout=1, delay=0.1) -> bool: start_time = time.time() + last_error = None while time.time() - start_time < timeout: if not self.is_running(): if self.process is None: logger.error("Server process was never started") else: returncode = self.process.poll() logger.error("Server process exited with code: %d", returncode) return False try: with socket.create_connection( ("localhost", self.port), timeout=request_timeout ): return True except (socket.timeout, ConnectionRefusedError, socket.gaierror, OSError) as ex: + last_error = ex logger.debug(f"Server not ready yet: {ex}") time.sleep(delay) + logger.error("Server failed to start within %d seconds. Last error: %s", timeout, last_error) return False
94-121: Consider adding port availability check.The server initialization could check if the port is available before starting:
def __init__(self, target: str, port: int = 5042, limit_max_requests: int = None): # ... validation code ... if not isinstance(port, int) or port < 1: raise ValueError("Port must be a positive integer") + # Check if port is available + try: + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: + s.bind(('localhost', port)) + except OSError: + raise ValueError(f"Port {port} is already in use")responder/ext/cli.py (1)
63-84: Consider enhancing build feedback.The build command implementation is secure and robust. Consider adding progress indicators:
try: + logger.info("Starting frontend asset build...") # # S603, S607 are addressed by validating the target directory. subprocess.check_call( # noqa: S603, S607 [npm_cmd, "run", "build"], cwd=target_path, timeout=300, ) + logger.info("Frontend asset build completed successfully") except FileNotFoundError:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/workflows/test.yamlis excluded by!**/*.yamlpyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (13)
.gitignore(1 hunks)README.md(1 hunks)docs/source/cli.rst(1 hunks)docs/source/index.rst(1 hunks)examples/helloworld.py(1 hunks)responder/__init__.py(1 hunks)responder/api.py(1 hunks)responder/ext/cli.py(1 hunks)responder/util/cmd.py(1 hunks)responder/util/python.py(1 hunks)setup.py(2 hunks)tests/test_cli.py(1 hunks)tests/util.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- responder/init.py
- .gitignore
- docs/source/index.rst
- examples/helloworld.py
- responder/api.py
- docs/source/cli.rst
- README.md
🧰 Additional context used
📓 Learnings (4)
tests/util.py (2)
Learnt from: amotl
PR: kennethreitz/responder#549
File: tests/util.py:0-0
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In the `wait_server_tcp` function in `tests/util.py`, include all relevant socket error numbers that represent transient errors to handle cross-platform differences and ensure robust error handling.
Learnt from: amotl
PR: kennethreitz/responder#549
File: tests/util.py:0-0
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In the `tests/util.py` file, when handling socket errors in the `wait_server_tcp` function, include Windows-specific error code `10035` (WSAEWOULDBLOCK) along with `errno.ECONNREFUSED` and `errno.ETIMEDOUT` to properly handle expected socket errors on Windows systems.
responder/ext/cli.py (2)
Learnt from: amotl
PR: kennethreitz/responder#549
File: responder/ext/cli.py:99-101
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In the Responder framework, error checking for API loading is already implemented within the `load_target` function in `responder/util/python.py`. Additional error handling in the CLI is unnecessary.
Learnt from: amotl
PR: kennethreitz/responder#549
File: responder/cli.py:76-78
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In `responder/cli.py`, the `load_target()` function already includes error handling for API server initialization, handling import errors and attribute errors when loading the target module and its properties. Therefore, additional error handling in the `cli()` function after calling `load_target()` is unnecessary.
tests/test_cli.py (5)
Learnt from: amotl
PR: kennethreitz/responder#549
File: tests/test_cli.py:153-159
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In the Responder codebase (Python), the `wait_server_tcp` function in `tests/util.py` already implements error checking based on socket error numbers. Avoid suggesting additional error handling for socket errors in tests that use this function, such as `test_cli_run` in `tests/test_cli.py`.
Learnt from: amotl
PR: kennethreitz/responder#549
File: tests/test_cli.py:153-159
Timestamp: 2024-11-12T09:17:41.000Z
Learning: Programming suggestions derived from inline comments may not be applicable or practical; avoid providing such suggestions in future reviews.
Learnt from: amotl
PR: kennethreitz/responder#549
File: tests/test_cli.py:67-86
Timestamp: 2024-11-12T09:17:41.000Z
Learning: Input validation for the `responder_build` function is already handled in `cli.py`, so we should avoid adding duplicate validation in `tests/test_cli.py`.
Learnt from: amotl
PR: kennethreitz/responder#549
File: tests/test_cli.py:0-0
Timestamp: 2024-11-12T09:17:41.000Z
Learning: When testing server startup in scenarios where only a single request is allowed before the server shuts down, avoid relying on specific log messages or sending probing HTTP requests. Instead, check if the server's port is open to confirm readiness.
Learnt from: amotl
PR: kennethreitz/responder#549
File: tests/test_cli.py:148-148
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In tests where the server is configured to self-terminate after processing a single request, using an HTTP-based probing mechanism to check server readiness is not feasible, as it would consume the single allowed request and cause the test to fail. In such cases, using a fixed sleep duration is acceptable.
responder/util/cmd.py (4)
Learnt from: amotl
PR: kennethreitz/responder#549
File: responder/util/cmd.py:34-63
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In the Responder project, a timeout for the `responder build` command is already implemented in `responder/ext/cli.py`, so adding a timeout in `ResponderProgram.build` in `responder/util/cmd.py` is unnecessary.
Learnt from: amotl
PR: kennethreitz/responder#549
File: responder/util/cmd.py:34-63
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In `responder/util/cmd.py`, the `build` method doesn't need to capture `stdout` and `stderr` from the subprocess because `npm` output is already forwarded to the terminal.
Learnt from: amotl
PR: kennethreitz/responder#549
File: responder/util/cmd.py:102-123
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In the test suite, the function `wait_server_tcp` in `tests/util.py` provides server readiness detection.
Learnt from: amotl
PR: kennethreitz/responder#549
File: responder/util/cmd.py:0-0
Timestamp: 2024-11-12T09:17:41.000Z
Learning: When using the `ResponderServer` class in tests, intermittent stalling may occur on CI/GitHub Actions; further investigation is needed to resolve this issue.
🪛 Ruff (0.8.2)
setup.py
141-141: Line too long (97 > 90)
(E501)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Full: Python pypy3.10 on windows-latest
🔇 Additional comments (19)
tests/util.py (5)
1-6: LGTM! Well-documented module purpose.The module docstring clearly explains the purpose and functionality of the utility functions.
21-33: LGTM! Clean implementation of random port allocation.The function properly handles socket cleanup using a try-finally block and provides clear documentation.
36-56: LGTM! Robust cross-platform error handling.The function effectively handles both Unix and Windows socket error codes, with clear documentation and proper use of
@lru_cache.
59-100: LGTM! Comprehensive server readiness checking.The implementation includes:
- Proper timeout handling
- Socket error handling for cross-platform compatibility
- Clear error messages
103-135: LGTM! Well-structured HTTP server checking.The implementation includes:
- Proper timeout and delay configuration
- Clear error messages with attempt counts
- Efficient request handling
responder/util/python.py (3)
13-23: LGTM! Well-defined custom exception.The
InvalidTargetexception includes clear documentation about its purpose and usage scenarios.
25-92: LGTM! Comprehensive target loading implementation.The function includes:
- Thorough input validation
- Clear error messages
- Proper attribute checking
- Comprehensive error handling
94-133: LGTM! Robust module loading with proper cleanup.The implementation includes:
- File extension validation
- Unique module naming using UUID
- Proper cleanup in sys.modules
- Specific error handling for different scenarios
setup.py (1)
117-117: LGTM! Proper CLI entry point configuration.The entry point correctly maps the
respondercommand to the CLI implementation.tests/test_cli.py (5)
1-14: LGTM! Clear test module documentation.The module docstring effectively documents:
- Test coverage scope
- Requirements
- Expected application behavior
36-46: LGTM! Environment-configurable timeouts.Good practice to make timeouts configurable via environment variables for CI/CD flexibility.
113-135: LGTM! Comprehensive build error testing.The parameterized test effectively covers various invalid JSON scenarios with clear test IDs.
153-159: LGTM! Well-documented flaky test configuration.The comment clearly explains:
- Why the test is marked as flaky
- Platform-specific error codes
- Expected error messages
194-212: LGTM! Thorough server lifecycle verification.The implementation effectively:
- Defines expected lifecycle messages
- Verifies message order
- Provides clear failure messages
responder/util/cmd.py (1)
1-18: Well-documented security considerations!The security documentation clearly outlines the key security measures for subprocess usage, and the imports are well-organized.
responder/ext/cli.py (4)
1-46: Well-structured documentation and imports!The docstring is comprehensive and follows best practices, with clear sections for commands, usage, options, arguments, and examples. Import organization is clean and logical.
48-61: Clean implementation with proper type hints!The CLI function is well-structured with clear documentation and appropriate type hints.
85-114: Robust implementation with proper validation!The run command implementation includes thorough validation and error handling. The use of
load_targetfor API loading is appropriate as it already includes comprehensive error handling.
117-127: Simple and effective logging configuration!The logging setup is straightforward and meets the current needs of the application.
amotl
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
responder/util/cmd.py (1)
22-25: Consider enhancing the class docstring.The current docstring could be more descriptive by including:
- The purpose of the class beyond just providing the path
- Available methods and their high-level functionality
- Usage examples
class ResponderProgram: """ - Provide full path to the `responder` program. + Utility class for managing Responder program execution. + + This class provides methods for: + - Locating the responder executable in PATH + - Building frontend assets using npm + + Example: + >>> program_path = ResponderProgram.path() + >>> build_status = ResponderProgram.build(Path("app_dir")) """responder/ext/cli.py (1)
63-86: Consider adding build progress feedback.While the build command implementation is secure and handles errors well, it could benefit from more user feedback:
- Log when build starts
- Log when build completes successfully
if build: target_path = Path(target).resolve() if target else Path.cwd() if not target_path.is_dir() or not (target_path / "package.json").exists(): logger.error(f"Invalid target directory or missing package.json: {target_path}") sys.exit(1) npm_cmd = "npm.cmd" if platform.system() == "Windows" else "npm" try: + logger.info("Starting frontend asset build") # S603, S607 are addressed by validating the target directory. subprocess.check_call( # noqa: S603, S607 [npm_cmd, "run", "build"], cwd=target_path, timeout=300, ) + logger.info("Frontend asset build completed successfully") except FileNotFoundError: logger.error("npm not found. Please install Node.js and npm.") sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/test.yamlis excluded by!**/*.yaml
📒 Files selected for processing (6)
responder/__init__.py(1 hunks)responder/api.py(2 hunks)responder/ext/cli.py(1 hunks)responder/util/cmd.py(1 hunks)setup.py(2 hunks)tests/test_cli.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- responder/init.py
- responder/api.py
- setup.py
🧰 Additional context used
📓 Learnings (3)
responder/ext/cli.py (2)
Learnt from: amotl
PR: kennethreitz/responder#549
File: responder/ext/cli.py:99-101
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In the Responder framework, error checking for API loading is already implemented within the `load_target` function in `responder/util/python.py`. Additional error handling in the CLI is unnecessary.
Learnt from: amotl
PR: kennethreitz/responder#549
File: responder/cli.py:76-78
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In `responder/cli.py`, the `load_target()` function already includes error handling for API server initialization, handling import errors and attribute errors when loading the target module and its properties. Therefore, additional error handling in the `cli()` function after calling `load_target()` is unnecessary.
tests/test_cli.py (5)
Learnt from: amotl
PR: kennethreitz/responder#549
File: tests/test_cli.py:153-159
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In the Responder codebase (Python), the `wait_server_tcp` function in `tests/util.py` already implements error checking based on socket error numbers. Avoid suggesting additional error handling for socket errors in tests that use this function, such as `test_cli_run` in `tests/test_cli.py`.
Learnt from: amotl
PR: kennethreitz/responder#549
File: tests/test_cli.py:153-159
Timestamp: 2024-11-12T09:17:41.000Z
Learning: Programming suggestions derived from inline comments may not be applicable or practical; avoid providing such suggestions in future reviews.
Learnt from: amotl
PR: kennethreitz/responder#549
File: tests/test_cli.py:67-86
Timestamp: 2024-11-12T09:17:41.000Z
Learning: Input validation for the `responder_build` function is already handled in `cli.py`, so we should avoid adding duplicate validation in `tests/test_cli.py`.
Learnt from: amotl
PR: kennethreitz/responder#549
File: tests/test_cli.py:0-0
Timestamp: 2024-11-12T09:17:41.000Z
Learning: When testing server startup in scenarios where only a single request is allowed before the server shuts down, avoid relying on specific log messages or sending probing HTTP requests. Instead, check if the server's port is open to confirm readiness.
Learnt from: amotl
PR: kennethreitz/responder#549
File: tests/test_cli.py:148-148
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In tests where the server is configured to self-terminate after processing a single request, using an HTTP-based probing mechanism to check server readiness is not feasible, as it would consume the single allowed request and cause the test to fail. In such cases, using a fixed sleep duration is acceptable.
responder/util/cmd.py (4)
Learnt from: amotl
PR: kennethreitz/responder#549
File: responder/util/cmd.py:34-63
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In the Responder project, a timeout for the `responder build` command is already implemented in `responder/ext/cli.py`, so adding a timeout in `ResponderProgram.build` in `responder/util/cmd.py` is unnecessary.
Learnt from: amotl
PR: kennethreitz/responder#549
File: responder/util/cmd.py:34-63
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In `responder/util/cmd.py`, the `build` method doesn't need to capture `stdout` and `stderr` from the subprocess because `npm` output is already forwarded to the terminal.
Learnt from: amotl
PR: kennethreitz/responder#549
File: responder/util/cmd.py:102-123
Timestamp: 2024-11-12T09:17:41.000Z
Learning: In the test suite, the function `wait_server_tcp` in `tests/util.py` provides server readiness detection.
Learnt from: amotl
PR: kennethreitz/responder#549
File: responder/util/cmd.py:0-0
Timestamp: 2024-11-12T09:17:41.000Z
Learning: When using the `ResponderServer` class in tests, intermittent stalling may occur on CI/GitHub Actions; further investigation is needed to resolve this issue.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Full: Python pypy3.10 on windows-latest
- GitHub Check: Full: Python 3.13 on windows-latest
- GitHub Check: Full: Python 3.12 on windows-latest
- GitHub Check: Full: Python 3.9 on windows-latest
- GitHub Check: Full: Python 3.8 on windows-latest
- GitHub Check: Full: Python 3.6 on windows-latest
- GitHub Check: Full: Python 3.13 on macos-latest
- GitHub Check: Full: Python 3.12 on macos-latest
- GitHub Check: Full: Python 3.9 on macos-latest
- GitHub Check: Full: Python 3.8 on macos-latest
- GitHub Check: Full: Python 3.7 on macos-13
- GitHub Check: Full: Python pypy3.10 on ubuntu-20.04
🔇 Additional comments (19)
responder/util/cmd.py (7)
1-20: Well-documented security considerations!The security comments clearly explain the rationale behind the noqa directives and outline the security measures taken for subprocess usage.
27-42: Excellent implementation of the path method!The implementation includes:
- Proper caching using
@functools.lru_cache- Platform-specific executable name handling
- Informative error message with installation instructions and searched paths
44-73: Well-structured build method with comprehensive validation!The implementation includes:
- Thorough input validation
- Well-documented API with Args, Returns, and Raises sections
- Clear error messages
76-131: Excellent class design and initialization!The implementation includes:
- Comprehensive docstring with description, args, and examples
- Thorough input validation
- Port availability check
- Proper signal handler setup
- Thread safety with process lock
132-153: Well-implemented run method with proper environment handling!The implementation includes:
- Correct command construction
- Environment preservation
- Thread-safe process management
155-187: Robust process termination implementation!The implementation includes:
- Protection against multiple stop attempts
- Graceful shutdown with timeout
- Proper signal handling
- Informative logging
189-234: Comprehensive server status monitoring!The implementation includes:
- Detailed server readiness checking
- Proper error handling and logging
- Process status verification
However, since intermittent stalling occurs on CI/GitHub Actions, consider adding more detailed logging about the process state during the readiness check:
responder/ext/cli.py (3)
1-44: Well-structured docstring and imports!The docstring provides comprehensive documentation with clear sections for commands, usage, options, and examples. The imports are well-organized and necessary for the functionality.
87-116: LGTM! Proper error handling for the run command.The run command implementation:
- Validates required target argument
- Handles limit_max_requests validation
- Uses load_target for API initialization
- Includes proper error handling
119-129: LGTM! Simple but effective logging configuration.The logging setup includes all necessary components (timestamp, logger name, level, message) and adjusts the log level based on debug mode.
tests/test_cli.py (9)
1-34: LGTM! Clear test documentation and dependencies.The module docstring clearly outlines:
- Test coverage for each CLI command
- Requirements and dependencies
- Expected application behavior
36-47: LGTM! Environment-configurable test settings.Test timeouts and delays are:
- Configurable via environment variables
- Have reasonable default values
- Include clear comments explaining their purpose
49-64: LGTM! Clear version test implementation.The test:
- Executes the version command
- Handles subprocess errors
- Verifies the output matches package version
67-86: LGTM! Well-documented build helper function.The helper function:
- Has clear type hints
- Includes comprehensive docstring
- Returns captured output for verification
88-101: LGTM! Clear success case test.The test:
- Creates a valid package.json
- Verifies build output
103-111: LGTM! Error case handling for missing package.json.The test verifies the appropriate error message when package.json is missing.
113-151: LGTM! Comprehensive invalid JSON test cases.The parameterized test:
- Covers various JSON error scenarios
- Has clear test case identifiers
- Verifies appropriate error messages
153-159: LGTM! Well-documented flaky test configuration.The test:
- Is marked as flaky with retries
- Documents platform-specific error codes
160-212: LGTM! Robust server test implementation.The test:
- Uses proper server setup and cleanup
- Verifies HTTP response
- Checks server lifecycle messages in order
Note: This appears to be the first integration test for Responder, including its CLI interface, end-to-end.
About
The CLI interface got lost. This patch intends to bring it back.
After refactoring, the CLI module is living at
responder.ext.clinow.Setup
From now on, the CLI interface is optional, and its
docopt-ngdependency needs to be installed explicitly.pip install --upgrade 'responder[cli]'Usage
The feature will also sport a dedicated documentation page now, see preview at Responder CLI.
Software Tests
Previously, the CLI module apparently had no software tests. Now, the two major subcommand operations
buildvs.runare covered, mostly on their happy paths.