Uh oh!
There was an error while loading. Please reload this page.
bpo-26187: Tests for sqlite3 trace callback - #434
Conversation
mention-bot
commented
Mar 3, 2017
@AnishShah, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Yhg1s, @ghaering, @berkerpeksag, @mdickinson and @serhiy-storchaka to be potential reviewers. |
| "Unicode data %s garbled in trace callback: %s" | ||
| % (ascii(unicode_value), ', '.join(map(ascii, traced_statements)))) | ||
| def CheckTraceCallbackContent(self): |
There was a problem hiding this comment.
We should skip the test if sqlite3_prepare_v2 is not available.
There was a problem hiding this comment.
The test name doesn't explain what we actually testing here.
| def CheckTraceCallbackContent(self): | ||
| """ | ||
| Test that the statement are correct. Fix for bpo-26187 |
| con.execute(query) | ||
| con.commit() | ||
| queries.insert(1, "BEGIN ") | ||
| queries.append("COMMIT") |
There was a problem hiding this comment.
These two lines can be removed. We only want to test that set_trace_callback doesn't produce any duplicate entries.
palaviv
commented
Mar 4, 2017
berkerpeksag
commented
Mar 6, 2017
Thanks, Aviv. Next time please send your feedback as a comment and give the PR author some time to address your comments. Otherwise, we will end up with duplicate PRs. Also note that we always locally run tests and make sure that the test fails before the fix is applied so the chance of merging a broken test is minimal. |
palaviv
commented
Mar 10, 2017
Sorry about that @berkerpeksag. I actually started by writing a comment on this PR but found out that what I do is to write the all test again. I am still having hard time understanding what @AnishShah did here as I wrote a test case in the issue. |
No description provided.