Skip to content
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

Test against pysqlite3 running SQLite 3.37 #347

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

simonw
Copy link
Owner

@simonw simonw commented Nov 29, 2021

Refs #346 and #344.

@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

Merging #347 (71b6c38) into main (213a0ff) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 71b6c38 differs from pull request most recent head 1a7ef2f. Consider uploading reports for the commit 1a7ef2f to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main     #347   +/-   ##
=======================================
  Coverage   96.51%   96.52%           
=======================================
  Files           5        5           
  Lines        2270     2271    +1     
=======================================
+ Hits         2191     2192    +1     
  Misses         79       79           
Impacted Files Coverage Δ
sqlite_utils/cli.py 95.73% <100.00%> (ø)
sqlite_utils/utils.py 93.68% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 213a0ff...1a7ef2f. Read the comment docs.

@simonw
Copy link
Owner Author

simonw commented Nov 29, 2021

strategy:
matrix:
python-version: ["3.6", "3.7", "3.8", "3.9", "3.10"]
numpy: [0, 1]
os: [ubuntu-latest, macos-latest, windows-latest]
include:
- os: ubuntu-latest
python-version: "3.10"
numpy: 0
pysqlite3_sqlite_3_37: 1

This configuration means that the numpy=0, Python=3.10, os=Ubuntu build will additionally use pysqlite3 with the SQLite 3.37.0.

It's failing right now: https://github.com/simonw/sqlite-utils/runs/4360593156 - because pysqlite3 doesn't provide .iterdump().

I can use the workaround from this comment: coleifer/pysqlite3#24 (comment)

@simonw
Copy link
Owner Author

simonw commented Nov 29, 2021

Here's the test run that's installing pysqlite3 and that version of SQLite: https://github.com/simonw/sqlite-utils/runs/4360663292

@simonw
Copy link
Owner Author

simonw commented Nov 29, 2021

It failed on other Python versions with mypy:

sqlite_utils/utils.py:8: error: Cannot find implementation or library stub for module named "sqlite3.dump"
sqlite_utils/utils.py:8: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports

@simonw
Copy link
Owner Author

simonw commented Nov 29, 2021

Took a bit of experimenting to get both mypy AND flake8 to ignore the same line. The incantation that worked was this one:

from sqlite3.dump import _iterdump as iterdump # type: ignore # noqa: F401

Order here matters - this did NOT work for both tools:

 from sqlite3.dump import _iterdump as iterdump   # noqa: F401 # type: ignore

@simonw
Copy link
Owner Author

simonw commented Nov 29, 2021

Some interesting test failures in the version that runs with pysqlite3:

=========================== short test summary info ============================
FAILED tests/test_cli.py::test_enable_wal - assert 0 == 1
FAILED tests/test_cli.py::test_disable_wal - pysqlite3.dbapi2.OperationalErro...
FAILED tests/test_fts.py::test_rebuild_fts[searchable] - pysqlite3.dbapi2.Dat...
FAILED tests/test_fts.py::test_rebuild_fts[searchable_fts] - pysqlite3.dbapi2...
FAILED tests/test_wal.py::test_enable_disable_wal - pysqlite3.dbapi2.Operatio...
================== 5 failed, 750 passed, 3 skipped in 15.20s ===================

https://github.com/simonw/sqlite-utils/runs/4360759085

The WAL errors look like this:

E           pysqlite3.dbapi2.OperationalError: cannot change into wal mode from within a transaction

Triggered by a call to db.enable_wal()

The FTS errors are caused by tests that try to deliberately corrupt the FTS index by running fresh_db["searchable_fts_data"].delete_where() - and then rebuilding it using rebuild_fts():

    @pytest.mark.parametrize("table_to_fix", ["searchable", "searchable_fts"])
    def test_rebuild_fts(fresh_db, table_to_fix):
        table = fresh_db["searchable"]
        table.insert(search_records[0])
        table.enable_fts(["text", "country"])
        # Run a search
        rows = list(table.search("tanuki"))
        assert len(rows) == 1
        assert {
            "rowid": 1,
            "text": "tanuki are running tricksters",
            "country": "Japan",
            "not_searchable": "foo",
        }.items() <= rows[0].items()
        # Delete from searchable_fts_data
        fresh_db["searchable_fts_data"].delete_where()
        # This should have broken the index
        with pytest.raises(sqlite3.DatabaseError):
            list(table.search("tanuki"))
        # Running rebuild_fts() should fix it
>       fresh_db[table_to_fix].rebuild_fts()

tests/test_fts.py:277: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
sqlite_utils/db.py:1947: in rebuild_fts
    self.db.execute(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <Database <pysqlite3.dbapi2.Connection object at 0x7fd95d299c30>>
sql = "INSERT INTO [searchable_fts]([searchable_fts]) VALUES('rebuild');"
parameters = None

    def execute(
        self, sql: str, parameters: Optional[Union[Iterable, dict]] = None
    ) -> sqlite3.Cursor:
        "Execute SQL query and return a ``sqlite3.Cursor``."
        if self._tracer:
            self._tracer(sql, parameters)
        if parameters is not None:
            return self.conn.execute(sql, parameters)
        else:
>           return self.conn.execute(sql)
E           pysqlite3.dbapi2.DatabaseError: database disk image is malformed

@simonw
Copy link
Owner Author

simonw commented Nov 29, 2021

A short term fix would be to skip those tests against pysqlite3 - but longer term it would be good to address the underlying issue, particularly for the WAL ones (the FTS ones aren't too worrying since if you deliberately try and break the FTS table it's not hugely problematic if you corrupt your database).

@simonw
Copy link
Owner Author

simonw commented Nov 29, 2021

If I'm going to skipIf() those tests I need a way to check if pysqlite3 is being used.

@simonw
Copy link
Owner Author

simonw commented Dec 11, 2021

The change I made to that test in #354 might help with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant