pwnshop: per-test timeout - #84
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85e5663677
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| except subprocess.TimeoutExpired as e: | ||
| logger.warning("test %s timed out after %ds in %s", test_name, test_timeout, challenge_path) | ||
| results.append((test_name, False, f"TIMEOUT after {test_timeout}s\n{(e.stdout or b'').decode(errors='replace')}")) |
There was a problem hiding this comment.
When subprocess.run(..., text=True) times out, TimeoutExpired.stdout is already a str (or None), so calling .decode(...) raises AttributeError. That means a timed-out test will crash the runner instead of being recorded as a failure. This only occurs on timeout paths, but it prevents the new --test-timeout behavior from working reliably. Consider using e.stdout or "" without decoding (or decoding only if it's bytes).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds per-test timeout functionality to the pwnshop testing framework. When tests exceed the specified timeout duration, they are gracefully terminated and marked as failed, allowing the test suite to continue with remaining tests.
Changes:
- Added
--test-timeoutcommand-line option to specify timeout in seconds for individual tests - Implemented timeout handling with proper exception catching and logging
- Enhanced error handling to include
subprocess.CalledProcessError - Made container cleanup more robust by removing
check=Trueand addingstderr=subprocess.DEVNULLto the docker kill command
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tools/pwnshop/src/pwnshop/commands/test.py | Adds --test-timeout CLI option, implements timeout exception handling for test execution, and adds subprocess.CalledProcessError to error handling |
| tools/pwnshop/src/pwnshop/lib/init.py | Makes container cleanup more robust by removing check=True and adding stderr=subprocess.DEVNULL to suppress errors during cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pwnshop: fix TimeoutExpired handling with text=True When subprocess.run is called with text=True, TimeoutExpired.stdout is already a string, not bytes. Remove the incorrect .decode() call that would raise AttributeError on timeout. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> test
… to docker run. This is what claude seems to expect.
1bd515f to
5901212
Compare
|
@zardus To make sure I understand the goal here, it's basically that you want to be able to run tests locally while developing, and lets say you run tests for the entire module (which might be expected to take 5+ minutes, but you'd be surprised if an individual test took more than 15 seconds): this feature allows you to assert that while developing? But this isn't a goal for CI purposes? Would better (clean) output also help? Like if you could see in the progress bar 7 challenges have finished, and the current running challenge has been going for a while? |
No description provided.