gh-108550: Speed up sqlite3 tests #108551

erlend-aasland · 2023-08-27T21:33:59Z

Refactor the CLI so we can easily invoke it and mock command-line
arguments. Adapt the CLI tests so we no longer have to launch a
separate process.

Disable the busy handler for all concurrency tests; we have full
control over the order of the SQLite C API calls, so we can safely
do this.

The sqlite3 test suite now completes ~8 times faster than before.

Issue: Speed up sqlite3 tests #108550

Refactor the CLI so we can easily invoke it and mock command-line arguments. Adapt the CLI tests so we no longer have to launch a separate process. Disable the busy handler for all concurrency tests; we have full control over the order of the SQLite C API calls, so we can safely do this. The sqlite3 test suite now completes ~10 times faster than before.

erlend-aasland · 2023-08-27T21:36:26Z

I ran this some hundred times locally:

$ ./python.exe -m test test_sqlite3 -F
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 4.23 Run tests sequentially
0:00:00 load avg: 4.23 [  1] test_sqlite3
Warning -- files was modified by test_sqlite3
Warning --   Before: []
Warning --   After:  ['default.profraw'] 
Warning -- files was modified by test_sqlite3
Warning --   Before: []
Warning --   After:  ['default.profraw'] 
0:00:00 load avg: 4.23 [  2/1] test_sqlite3 -- test_sqlite3 failed (env changed)
[...]
0:03:25 load avg: 5.57 [694/1] test_sqlite3

(env changes is because I have profiling enabled by default on my local build)

erlend-aasland · 2023-08-27T21:38:42Z

@AlexWaygood, want to have a quick look at the __main__.py and test_cli change? The latter could use a refactor, but I'm not sure it is worth the churn.

erlend-aasland · 2023-08-27T21:38:53Z

cc. @serhiy-storchaka if you want to have a look

serhiy-storchaka

Nice.

But it was possible to rewrite InteractiveSession tests in such way because you do not test interactivity. For example, that the prompt is displayed before any input. That the continuation line prompt is displayed before finishing the multiline command. In future we can build readline support in the REPL and add tests for history or completion.

Is it possible to support multiple communicate()?

erlend-aasland · 2023-08-28T04:23:48Z

Yeah, InteractiveSession had flaws, and it is suboptimal currently. I thought realine support was baked into the interactive console class from code, but I may be misremembering.

erlend-aasland · 2023-08-28T04:25:46Z

Is it possible to support multiple communicate()?

It should be possible, but I think that is out of scope for this PR.

serhiy-storchaka · 2023-08-28T04:46:47Z

On other hand, it is not necessary. There is already a test for initial prompt. We can just add a new test for incomplete multiline input and check secondary prompts. See #108556.

erlend-aasland · 2023-08-28T06:43:36Z

On other hand, it is not necessary. There is already a test for initial prompt. We can just add a new test for incomplete multiline input and check secondary prompts. See #108556.

We already do, no? test_interact_valid_multiline_sql checks for PS2.

serhiy-storchaka

There are many ways to break the interactive session, but keep the changed tests passed:

Read input not line by line, by the whole content. It delays any interaction until stdin be closed.
Buffer stdout, or stderr, or both until the program ends.

So I am not sure about this change. Maybe simulating a subprocess with a thread can mitigate the problem, but we need an analogue of blocking PipeInputStream for this.

There are no such problems with non-interactive tests.

Lib/test/test_sqlite3/test_transactions.py

Lib/test/test_sqlite3/test_cli.py

Lib/sqlite3/__main__.py

Co-authored-by: Serhiy Storchaka <[email protected]>

erlend-aasland · 2023-08-28T12:07:10Z

This PR seems to risk running stale. I'm opening a more narrowed PR for the busy timeout optimisation instead.

serhiy-storchaka

I approve this change, but I afraid that we will need to revert some of them in future.

erlend-aasland · 2023-08-28T12:16:46Z

I approve this change, but I afraid that we will need to revert some of them in future.

Yes, I also believe we need to further rework the interactive tests. I'm not even sure we need all of them.

erlend-aasland · 2023-08-28T12:16:57Z

Thanks for the review, Serhiy.

miss-islington · 2023-08-28T12:17:45Z

Sorry, @erlend-aasland, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 0e8b3fc718c8a1c4de558c553d9e05049c1dbec6 3.11

Refactor the CLI so we can easily invoke it and mock command-line arguments. Adapt the CLI tests so we no longer have to launch a separate process. Disable the busy handler for all concurrency tests; we have full control over the order of the SQLite C API calls, so we can safely do this. The sqlite3 test suite now completes ~8 times faster than before. (cherry picked from commit 0e8b3fc) Co-authored-by: Erlend E. Aasland <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-bot · 2023-08-28T12:17:48Z

GH-108566 is a backport of this pull request to the 3.12 branch.

Disable the busy handler for all concurrency tests; we have full control over the order of the SQLite C API calls, so we can safely do this. test_sqlite3.test_transactions now completes ~10 times faster than before. Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-bot · 2023-08-28T12:21:21Z

GH-108567 is a backport of this pull request to the 3.11 branch.

AlexWaygood

Post-merge review: LGTM too (though I'm no expert on testing interactive REPLs)!

Execution time for me on Windows went down from 3.8s to 548ms :D

erlend-aasland · 2023-08-28T12:24:11Z

Execution time for me on Windows went down from 3.8s to 548ms :D

Yay! 😃

Disable the busy handler for all concurrency tests; we have full control over the order of the SQLite C API calls, so we can safely do this. test_sqlite3.test_transactions now completes ~10 times faster than before. Co-authored-by: Serhiy Storchaka <[email protected]>

gh-108550: Speed up sqlite3 tests (GH-108551) Refactor the CLI so we can easily invoke it and mock command-line arguments. Adapt the CLI tests so we no longer have to launch a separate process. Disable the busy handler for all concurrency tests; we have full control over the order of the SQLite C API calls, so we can safely do this. The sqlite3 test suite now completes ~8 times faster than before. (cherry picked from commit 0e8b3fc) Co-authored-by: Erlend E. Aasland <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]>

…honGH-108618) (cherry picked from commit c884784) Co-authored-by: Erlend E. Aasland <[email protected]>

… (#108621) gh-108550: Fix sqlite3 CLI regression from gh-108551 (GH-108618) (cherry picked from commit c884784) Co-authored-by: Erlend E. Aasland <[email protected]>

erlend-aasland requested a review from berkerpeksag as a code owner August 27, 2023 21:33

erlend-aasland added skip news needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Aug 27, 2023

bedevere-bot mentioned this pull request Aug 27, 2023

Speed up sqlite3 tests #108550

Closed

bedevere-bot added the awaiting core review label Aug 27, 2023

serhiy-storchaka reviewed Aug 28, 2023

View reviewed changes

erlend-aasland mentioned this pull request Aug 28, 2023

gh-108617: Extend interactive session tests for sqlite3 #108556

Merged

serhiy-storchaka reviewed Aug 28, 2023

View reviewed changes

Lib/test/test_sqlite3/test_transactions.py Show resolved Hide resolved

Lib/test/test_sqlite3/test_cli.py Outdated Show resolved Hide resolved

Lib/sqlite3/__main__.py Outdated Show resolved Hide resolved

erlend-aasland and others added 3 commits August 28, 2023 10:25

Update Lib/test/test_sqlite3/test_cli.py

c744063

Co-authored-by: Serhiy Storchaka <[email protected]>

Pull in main

9bd7615

Use *args iso. *argv

18eefdb

erlend-aasland marked this pull request as draft August 28, 2023 12:07

bedevere-bot removed the awaiting core review label Aug 28, 2023

serhiy-storchaka approved these changes Aug 28, 2023

View reviewed changes

bedevere-bot added the awaiting merge label Aug 28, 2023

erlend-aasland marked this pull request as ready for review August 28, 2023 12:15

bedevere-bot added awaiting core review and removed awaiting merge labels Aug 28, 2023

erlend-aasland merged commit 0e8b3fc into python:main Aug 28, 2023

This comment was marked as outdated.

Sign in to view

bedevere-bot removed the awaiting core review label Aug 28, 2023

erlend-aasland deleted the sqlite/speed-up-tests branch August 28, 2023 12:17

miss-islington assigned erlend-aasland Aug 28, 2023

bedevere-bot removed the needs backport to 3.12 only security fixes label Aug 28, 2023

bedevere-bot removed the needs backport to 3.11 only security fixes label Aug 28, 2023

AlexWaygood reviewed Aug 28, 2023

View reviewed changes

erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request Aug 29, 2023

pythongh-108550: Fix sqlite3 CLI regression from pythongh-108551

3b3a65c

erlend-aasland added a commit that referenced this pull request Aug 29, 2023

gh-108550: Fix sqlite3 CLI regression from gh-108551 (#108618)

c884784

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 29, 2023

pythongh-108550: Fix sqlite3 CLI regression from pythongh-108551 (pyt…

a9af6fe

…honGH-108618) (cherry picked from commit c884784) Co-authored-by: Erlend E. Aasland <[email protected]>

Uh oh!

gh-108550: Speed up sqlite3 tests #108551

gh-108550: Speed up sqlite3 tests #108551

Uh oh!

Conversation

erlend-aasland commented Aug 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented Aug 27, 2023

Uh oh!

erlend-aasland commented Aug 27, 2023

Uh oh!

erlend-aasland commented Aug 27, 2023

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented Aug 28, 2023

Uh oh!

erlend-aasland commented Aug 28, 2023

Uh oh!

serhiy-storchaka commented Aug 28, 2023

Uh oh!

erlend-aasland commented Aug 28, 2023

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erlend-aasland commented Aug 28, 2023

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented Aug 28, 2023

Uh oh!

erlend-aasland commented Aug 28, 2023

Uh oh!

This comment was marked as outdated.

miss-islington commented Aug 28, 2023

Uh oh!

bedevere-bot commented Aug 28, 2023

Uh oh!

bedevere-bot commented Aug 28, 2023

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

erlend-aasland commented Aug 28, 2023

Uh oh!

Uh oh!

erlend-aasland commented Aug 27, 2023 •

edited

Loading