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

Add compile option to Dockerfile to fix failing test (fixes #696) #1223

Merged
merged 1 commit into from
Mar 7, 2021

Conversation

bobwhitelock
Copy link
Contributor

This test was failing when run inside the Docker container: test_searchable[/fixtures/searchable.json?_search=te*+AND+do*&_searchmode=raw-expected_rows3],

with this error:

    def test_searchable(app_client, path, expected_rows):
        response = app_client.get(path)
>       assert expected_rows == response.json["rows"]
E       AssertionError: assert [[1, 'barry c...sel', 'puma']] == []
E         Left contains 2 more items, first extra item: [1, 'barry cat', 'terry dog', 'panther']
E         Full diff:
E         + []
E         - [[1, 'barry cat', 'terry dog', 'panther'],
E         -  [2, 'terry dog', 'sara weasel', 'puma']]

The issue was that the version of sqlite3 built inside the Docker container was built with FTS3 and FTS4 enabled, but without the
SQLITE_ENABLE_FTS3_PARENTHESIS compile option passed, which adds support for using AND and NOT within match expressions (see https://sqlite.org/fts3.html#compiling_and_enabling_fts3_and_fts4 and https://www.sqlite.org/compile.html).

Without this, the AND used in the search in this test was being interpreted as a literal string, and so no matches were found. Adding this compile option fixes this.


I actually ran into this issue because the same test was failing when I ran the test suite on my own machine, outside of Docker, and so I eventually tracked this down to my system sqlite3 also being compiled without this option.

I wonder if this is a sign of a slightly deeper issue, that Datasette can silently behave differently based on the version and compilation of sqlite3 it is being used with. On my own system I fixed the test suite by running pip install pysqlite3-binary, so that this would be picked up instead of the sqlite package, as this seems to be compiled using this option, . Maybe using pysqlite3-binary could be installed/recommended by default so a more deterministic version of sqlite is used? Or there could be some feature detection done on the available sqlite version, to know what features are available and can be used/tested?

This test was failing when run inside the Docker container:
`test_searchable[/fixtures/searchable.json?_search=te*+AND+do*&_searchmode=raw-expected_rows3]`,

with this error:

```
    def test_searchable(app_client, path, expected_rows):
        response = app_client.get(path)
>       assert expected_rows == response.json["rows"]
E       AssertionError: assert [[1, 'barry c...sel', 'puma']] == []
E         Left contains 2 more items, first extra item: [1, 'barry cat', 'terry dog', 'panther']
E         Full diff:
E         + []
E         - [[1, 'barry cat', 'terry dog', 'panther'],
E         -  [2, 'terry dog', 'sara weasel', 'puma']]
```

The issue was that the version of sqlite3 built inside the Docker
container was built with FTS3 and FTS4 enabled, but without the
`SQLITE_ENABLE_FTS3_PARENTHESIS` compile option passed, which adds
support for using `AND` and `NOT` within `match` expressions (see
https://sqlite.org/fts3.html#compiling_and_enabling_fts3_and_fts4 and
https://www.sqlite.org/compile.html).

Without this, the `AND` used in the search in this test was being
interpreted as a literal string, and so no matches were found. Adding
this compile option fixes this.
@codecov
Copy link

codecov bot commented Feb 12, 2021

Codecov Report

Merging #1223 (d1cd1f2) into main (9603d89) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1223   +/-   ##
=======================================
  Coverage   91.42%   91.42%           
=======================================
  Files          32       32           
  Lines        3955     3955           
=======================================
  Hits         3616     3616           
  Misses        339      339           

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 9603d89...d1cd1f2. Read the comment docs.

@simonw
Copy link
Owner

simonw commented Mar 7, 2021

This is fantastic, thanks so much for tracking this down.

@simonw simonw merged commit d0fd833 into simonw:main Mar 7, 2021
@bobwhitelock bobwhitelock deleted the issue-696 branch March 7, 2021 12:01
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.

None yet

2 participants