Skip to content

bpo-45243: Use connection limits to simplify sqlite3 tests - #29356

Merged
serhiy-storchaka merged 13 commits into
python:mainfrom
erlend-aasland:sqlite-use-limits-in-tests
Nov 5, 2021
Merged

bpo-45243: Use connection limits to simplify sqlite3 tests#29356
serhiy-storchaka merged 13 commits into
python:mainfrom
erlend-aasland:sqlite-use-limits-in-tests

Conversation

@erlend-aasland

@erlend-aaslanderlend-aasland commented Nov 2, 2021

Copy link
Copy Markdown
Contributor

@erlend-aasland
erlend-aaslandforce-pushed the sqlite-use-limits-in-tests branch from 07d012a to 8cdb980CompareNovember 2, 2021 09:34
@erlend-aaslanderlend-aasland changed the title bpo-45243: Use sqlite3 connection limit context manager in test suitebpo-45243: Use connection limits to simplify sqlite3 testsNov 2, 2021
@erlend-aasland
erlend-aasland marked this pull request as ready for review November 2, 2021 09:58
@erlend-aaslanderlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 2, 2021
@bedevere-bot

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 22adb06 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 2, 2021

@serhiy-storchakaserhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bigmem tests rewriting does not look correct to me. These tests are specially purposed to test the integer overflow. They try with scripts of specific length (slightly below or above INT_MAX). It does not make sense to test scripts with smaller length. They are decorated with bigmemtest because they require a large amount of RAM and should be skipped if it is not enough.

Comment threadLib/test/test_sqlite3/test_userfunctions.py Outdated
Comment threadLib/test/test_sqlite3/test_dbapi.py
@erlend-aasland

erlend-aasland commented Nov 2, 2021

Copy link
Copy Markdown
ContributorAuthor

Bigmem tests rewriting does not look correct to me. These tests are specially purposed to test the integer overflow. [...]

The C code explicitly checks the current connection limit (SQLITE_LIMIT_LENGTH); it does not check against INT_MAX. The maximum limit is defined by the compile-time constant SQLITE_MAX_LENGTH, which defaults to 1_000_000_000 (and can be maximum 2**31-1). This limit can be altered using sqlite3_setlimit(db, SQLITE_LIMIT_LENGTH, new_limit) on the database connection. Using bigmem tests we of course ensure that we will hit the roof, but only if the test machine has enough RAM. Using custom connection limits in the test, we can lower the limit temporarily and ensure that we can run these tests on any machine.

executemany check:

size_tsql_len=strlen(sql_script);
intmax_length=sqlite3_limit(self->connection->db,
SQLITE_LIMIT_LENGTH, -1);
if (sql_len >= (unsigned)max_length) {
PyErr_SetString(self->connection->DataError,
"query string is too large");
returnNULL;
}

execute / create statement check:

sqlite3*db=connection->db;
intmax_length=sqlite3_limit(db, SQLITE_LIMIT_LENGTH, -1);
if (size >= max_length) {
PyErr_SetString(connection->DataError,
"query string is too large");
returnNULL;
}

@erlend-aasland

Copy link
Copy Markdown
ContributorAuthor

@serhiy-storchaka, let me know if you want me to revert the bigmem tests. I believe it is correct to change them, though (see my previous comment).

@serhiy-storchakaserhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let change these tests.

Comment threadLib/test/test_sqlite3/test_dbapi.py Outdated

@serhiy-storchakaserhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment threadLib/test/test_sqlite3/test_userfunctions.py Outdated
@erlend-aasland

Copy link
Copy Markdown
ContributorAuthor

Thanks for reviewing, @serhiy-storchaka. What do you think of my proposed patch in #29356 (comment)? I guess it is out of scope for this PR, but I think we should apply it.

@erlend-aasland

erlend-aasland commented Nov 4, 2021

Copy link
Copy Markdown
ContributorAuthor

FYI, I'll merge in main, in order to rerun the CI. (asyncio failed again)

@erlend-aasland

Copy link
Copy Markdown
ContributorAuthor

CI is ok; ready for merge :) I'll create an issue regarding the length checks.

Comment threadLib/test/test_sqlite3/test_userfunctions.py Outdated
Comment threadLib/test/test_sqlite3/test_dbapi.py Outdated
Comment threadLib/test/test_sqlite3/test_dbapi.py Outdated
build-in round is missing if SQLITE_OMIT_FLOATING_POINT is defined
Comment threadLib/test/test_sqlite3/test_dbapi.py Outdated
@serhiy-storchaka
serhiy-storchaka merged commit 3d42cd9 into python:mainNov 5, 2021
@erlend-aasland

Copy link
Copy Markdown
ContributorAuthor

Thank you for reviewing, Serhiy. The PR is in a better shape now.

@erlend-aasland
erlend-aasland deleted the sqlite-use-limits-in-tests branch November 5, 2021 17:27
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip newstestsTests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@erlend-aasland@bedevere-bot@serhiy-storchaka@the-knights-who-say-ni