-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix cstest_py and add negative tests. #2807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in cstest_py that prevented failing tests from being properly reported and adds negative test cases to prevent regression. It also addresses memory leaks that occurred when tests failed.
Key changes:
- Fixed
cstest_pyto return test failures correctly by replacing non-returning_check()function calls with direct conditional checks - Added memory cleanup functions to fix leaks in the C
cstestimplementation - Created six negative test cases to verify that test failures are properly detected
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| bindings/python/cstest_py/src/cstest_py/cstest.py | Fixed bug where comparison failures didn't return and added exit on inconsistent statistics |
| suite/cstest/src/test_case.c | Added cs_free() call in failure macro to prevent memory leaks |
| suite/cstest/src/cstest.c | Added cleanup_test_files() function and calls to free memory on exit paths |
| suite/cstest/CMakeLists.txt | Added six negative test cases marked with WILL_FAIL property |
| suite/run_tests.py | Added logic to run negative tests and verify they fail as expected |
| tests/negative/should_fail_*.yaml | Six new test case files designed to fail for different reasons |
| .github/workflows/CITest.yml | Updated CI to run negative tests and use more specific valgrind test paths |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Your checklist for this pull request
Detailed description
Fixes a bug in
cstest_pywhich didn't report failing tests.The C
cstestwas luckily not affected.It also fixes several small leaks which occur when tests fail.
Test plan
Because of this the PR adds negative tests so it won't happen again.
Closing issues
...