Add unit tests for utils/process_result.py with consolidated pytest CI#197
Add unit tests for utils/process_result.py with consolidated pytest CI#197Copilot wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive unit testing for utils/process_result.py and sets up automated CI testing via GitHub Actions. The implementation includes 15 test cases covering core functionality such as benchmark result processing, metric conversions (milliseconds to seconds, TPOT to interactivity), GPU calculations, and various configuration scenarios. The tests use dynamic path resolution and proper fixtures for better maintainability.
Key changes:
- Created
test_process_result.pywith 15 comprehensive unit tests - Added GitHub Actions workflow for automated pytest execution
- Implemented dynamic path resolution for better portability
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
utils/test_process_result.py |
New test file with 15 unit tests covering all major functionality of process_result.py |
.github/workflows/test-utils-process-result.yml |
GitHub Actions workflow configuration for running pytest on relevant file changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| jobs: | ||
| test: | ||
| if: github.event_name != 'pull_request' || github.event.pull_request.draft != true |
There was a problem hiding this comment.
[nitpick] The condition skips tests on draft PRs, but the PR title contains '[WIP]' suggesting this is a work-in-progress pull request. Consider removing this condition or updating the workflow to run on draft PRs to ensure tests are executed during development.
| if: github.event_name != 'pull_request' || github.event.pull_request.draft != true |
| try: | ||
| with patch.dict(os.environ, basic_env_vars): | ||
| # Import and execute the script | ||
| exec(open(PROCESS_RESULT_PATH).read()) |
There was a problem hiding this comment.
Using exec() with file contents is a security risk and makes the code harder to test and debug. Consider refactoring process_result.py to expose functions that can be imported and called directly, rather than executing the entire script.
There was a problem hiding this comment.
Refactored process_result.py to expose a process_benchmark_result() function that can be imported and tested directly. Tests now call the function instead of using exec(), which is more secure and maintainable. All 15 tests still pass. Changes in commit f0353fc.
|
@copilot can u consolidate this pytest with .github/workflows/test-matrix-logic.yml |
Co-authored-by: functionstackx <47992694+functionstackx@users.noreply.github.com>
…ixture Co-authored-by: functionstackx <47992694+functionstackx@users.noreply.github.com>
Co-authored-by: functionstackx <47992694+functionstackx@users.noreply.github.com>
0a7e283 to
a204048
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: functionstackx <47992694+functionstackx@users.noreply.github.com>
Adds comprehensive test coverage for the benchmark result processing script with automated CI verification. Refactors
process_result.pyto use testable functions instead of top-level script execution.Changes
utils/process_result.py: Refactored for better testabilityprocess_benchmark_result()function containing core processing logicmain()function for CLI executionutils/test_process_result.py: 15 unit tests covering:.github/workflows/test-utils.yml: Consolidated pytest CI workflow.gitignore: Added pytest cache and Python bytecode exclusionsTest Example
All 88 tests pass. No security alerts from CodeQL.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.