-
Notifications
You must be signed in to change notification settings - Fork 375
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
Fixed crash caused by non-freed reference #589
Fixed crash caused by non-freed reference #589
Conversation
You're not going to include a test for this PR? Perhaps the one that begins a transaction and execute an invalid query. Iterate 20 times. |
should have test for both PDO and SQLSRV to confirm it is fixed in PDO and doesn't exist in SQLSRV |
Tests that crash because of the bug would still output the expected results/errors because the crash occurs after script execution has finished. Also, a test that crashes can still pass when run under run-tests.php, and there is no crash then. So a test that iterates beginTransaction() and query() 20 times could output whatever we want it to 20 times and then crash at the end, and run-tests.php won't catch it. We may need to use manual verification for this fix in future. |
Codecov Report
@@ Coverage Diff @@
## dev #589 +/- ##
==========================================
+ Coverage 74.84% 74.86% +0.01%
==========================================
Files 50 50
Lines 14933 14933
==========================================
+ Hits 11177 11179 +2
+ Misses 3756 3754 -2
Continue to review full report at Codecov.
|
Should still include tests for "manual verification" in the future |
Tests for manual verification that are always skipped when testing automatically on Github? I'll upload tests for that. |
Fix for #434.
This change is