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

Adding MySQL support for db fixtures #10

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,17 @@ python:
- '3.6'

env:
- TEST_DATABASE_URL=postgresql://postgres@localhost:5432/pytest_test
- DB=pgsql TEST_DATABASE_URL=postgresql://postgres@localhost:5432/pytest_test
- DB=mysql TEST_DATABASE_URL=mysql+mysqldb://[email protected]/pytest_test

addons:
postgresql: '9.6'
mysql: '5.7'

before_script:
- sh -c "if [ '$DB' = 'postgres' ]; then psql -c 'DROP DATABASE IF EXISTS pytest_test;' -U postgres; fi"
- sh -c "if [ '$DB' = 'postgres' ]; then psql -c 'CREATE DATABASE pytest_test;' -U postgres; fi"
- sh -c "if [ '$DB' = 'mysql' ]; then mysql -e 'CREATE DATABASE IF NOT EXISTS pytest_test;'; fi"
Copy link
Owner

Choose a reason for hiding this comment

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

Why are these lines necessary? I thought that the database fixture in tests/_conftest.py was setting up the database for us.

Copy link
Author

Choose a reason for hiding this comment

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

Good point, I was learning a bit about travis when I copied these statements from the tutorial docs. Indeed they are being created in _conftest.py. I will remove them.


install:
- pip install --upgrade pip
Expand Down
3 changes: 3 additions & 0 deletions pytest_flask_sqlalchemy/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,9 @@ def _engine(pytestconfig, request, _transaction, mocker):
# the Engine dialect to reflect tables)
engine.dialect = connection.dialect

# Necessary for branching db test code
engine.name = connection.engine.name
Copy link
Owner

Choose a reason for hiding this comment

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

Nice, this is a clever way to introspect the database engine.


@contextlib.contextmanager
def begin():
'''
Expand Down
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def readme():
'pytest-mock>=1.6.2',
'SQLAlchemy>=1.2.2',
'Flask-SQLAlchemy>=2.3'],
extras_require={'tests': ['pytest-postgresql']},
extras_require={'tests': ['pytest-postgresql', 'pytest-mysql']},
classifiers=[
'Development Status :: 4 - Beta',
'Environment :: Plugins',
Expand Down
33 changes: 24 additions & 9 deletions tests/_conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import sqlalchemy as sa
from flask import Flask
from flask_sqlalchemy import SQLAlchemy
from pytest_mysql.factories import mysql
from pytest_postgresql.factories import (init_postgresql_database,
drop_postgresql_database)

Expand All @@ -17,7 +18,8 @@
'connection string to the environmental variable ' +
'TEST_DATABASE_URL in order to run tests.')
else:
DB_OPTS = sa.engine.url.make_url(DB_CONN).translate_connect_args()
DB_URL = sa.engine.url.make_url(DB_CONN)
DB_OPTS = DB_URL.translate_connect_args()

pytest_plugins = ['pytest-flask-sqlalchemy']

Expand All @@ -27,16 +29,28 @@ def database(request):
'''
Create a Postgres database for the tests, and drop it when the tests are done.
Copy link

@pmdarrow pmdarrow Mar 20, 2019

Choose a reason for hiding this comment

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

Suggested change
Create a Postgres database for the tests, and drop it when the tests are done.
Create a database for the tests, and drop it when the tests are done.

'''
pg_host = DB_OPTS.get("host")
pg_port = DB_OPTS.get("port")
pg_user = DB_OPTS.get("username")
pg_db = DB_OPTS["database"]
db_host = DB_OPTS.get("host")
db_port = DB_OPTS.get("port")
db_user = DB_OPTS.get("username")
db_name = DB_OPTS["database"]

init_postgresql_database(pg_user, pg_host, pg_port, pg_db)
BACKEND = DB_URL.get_backend_name()

@request.addfinalizer
def drop_database():
drop_postgresql_database(pg_user, pg_host, pg_port, pg_db, 9.6)
if BACKEND == 'mysql':
mysql('dummy_mysql_fixture', db=db_name)
Copy link
Owner

Choose a reason for hiding this comment

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

It seems like the mysql fixture factory creates a function-scoped fixture (docs), which means that it's dropping and recreating the database between every test. We should create a session-scoped version of the fixture to make sure that all tests are sharing the same database.

The docs suggest that the mysql_proc fixture factory will create a session-scoped fixture, but my read of the source code is that it's actually just creating a fixture corresponding to a MySQL server process. I'm not entirely sure whether there's a way to get it to create a session-scoped database.

It might be simpler to just use a Python MySQL client (like mysqlclient) to do this database creation/dropping work manually. I used pytest-postgresql to create the Postgres database because I was cargo culting from another library, but now that I read the pytest-postgresql source code I'm realizing that init_postgresql_database and drop_postgresql_database are really just running a couple of lines with psycopg2 to create and drop the database. (I wouldn't be surprised if it was similar problems with the pytest-postgreql fixture factories that caused us to use init_postgresql_database and drop_postgresql_database way back when. It's kind of weird that we're using library internals instead of the documented fixture factories.)

Let me know if you're having trouble with this and I'd be glad to give it a try! I don't have a lot of MySQL experience so I'll leave it up to you whether or not you want to take a stab at it.

Copy link
Author

Choose a reason for hiding this comment

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

Great notes, I'll confess I am very new to the pytest framework so I was overwhelmed with all the different interactions taking place. Having you clear up what needs to be done is helping me focus. Let me work on it a little and then we can have a look to refine it further.


elif BACKEND == 'postgresql':
init_postgresql_database(db_user, db_host, db_port, db_name)

@request.addfinalizer
def drop_database():
drop_postgresql_database(db_user, db_host, db_port, db_name, 9.6)

else:
raise ValueError(
'Unsupported database type ({}) requested in '
'TEST_DATABASE_URL: {}'.format(BACKEND, DB_URL)
)


@pytest.fixture(scope='session')
Expand All @@ -47,6 +61,7 @@ def app(database):
app = Flask(__name__)

app.config['SQLALCHEMY_DATABASE_URI'] = DB_CONN
app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = True
Copy link
Owner

Choose a reason for hiding this comment

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

What's your reasoning behind adding this config?

Copy link
Author

Choose a reason for hiding this comment

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

Oh yes, essentially SQLAlchemy pollutes the log with deprecation warning messages because default behaviour is changing. I noticed it when I was running pytest with the stdout capture disabled. Naturally if you run it normally you don't see it.

As to why I set it to True, just because it preserved existing behaviour. That being said, it is not required by the library, and disabling it/leaving it alone works fine. So I'll remove this statement as well.

Copy link
Owner

Choose a reason for hiding this comment

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

Great, thanks for the context! I'm glad you flagged this, we should address deprecations in a separate issue.


return app

Expand Down
52 changes: 38 additions & 14 deletions tests/test_fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,33 +212,57 @@ def test_drop_table(db_testdir):
'''
Make sure that we can drop tables and verify they do not exist in the context
of a test.

NOTE: For MySQL `DROP TABLE ...` statements "cause an implicit commit after
executing. The intent is to handle each such statement in its own special
transaction because it cannot be rolled back anyway."
https://dev.mysql.com/doc/refman/5.7/en/implicit-commit.html

So this test is skipped for 'mysql' engines
Copy link
Owner

Choose a reason for hiding this comment

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

What happens if you don't skip this test in MySQL? I would expect an autocommit not to cause a problem here, since the outermost transaction is supposed to listen for nested inner commits and restart transactions when they occur. Maybe there's something about "implicit commits" in MySQL that overrides nested transactions?

I'd like to get to the bottom of this behavior, since it seems pretty restricting to not be able to run any of the statements that are listed on the MySQL docs for implicit commits (DROP TABLE and TRUNCATE TABLE are used pretty often in the test suite that inspired this plugin, for example). Again, let me know if you get stuck and I'd be happy to help out with research.

Copy link
Author

@kamikaz1k kamikaz1k Jan 6, 2019

Choose a reason for hiding this comment

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

The reason I started looking into it was because the tests were failing to begin with, and I was really unaware as to why. I just thought to check that there might be a Transaction violation for DROP TABLE, and the MySQL docs is what I found in my search results.

Quoting from the docs (https://dev.mysql.com/doc/refman/5.7/en/implicit-commit.html):

The statements listed in this section (and any synonyms for them) implicitly end any transaction active in the current session, as if you had done a COMMIT before executing the statement.
Most of these statements also cause an implicit commit after executing. The intent is to handle each such statement in its own special transaction because it cannot be rolled back anyway.

And the way MySQL does nested transactions in the InnoDB engine is to use SAVEPOINT. If you use the COMMIT statement, it will commit the top level transaction.

Just to be sure, because I thought I might have misunderstood, I decided to just run a nested transaction manually. And they confirmed the behaviour observed.

That being said, it is reasonable to seek a more confident answer.

These are the statements I ran:

CREATE TABLE Test (id int primary key, some_value varchar(20));
SHOW TABLES;
# shows `Test` table is listed
SET autocommit = 0; # Enabled/Disabled does not seem to affect the results

START transaction;
# outer transaction has started

SAVEPOINT innertransaction;
# inner transaction is started -- this is how nested transactions are done in InnoDB

DROP TABLE Test;
SHOW TABLES;
# `Test` table is not listed

ROLLBACK TO innertransaction;
# > ERROR 1305 (42000): SAVEPOINT innertransaction does not exist

# the remaining lines below should have showed `Test` as recovered, but it does not
SHOW TABLES;

ROLLBACK;

Copy link
Author

Choose a reason for hiding this comment

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

I found a much more concrete reference in the MySQL docs:
https://dev.mysql.com/doc/refman/5.7/en/cannot-roll-back.html

Some statements cannot be rolled back. In general, these include data definition language (DDL) statements, such as those that create or drop databases, those that create, drop, or alter tables or stored routines.

Copy link
Owner

@jeancochrane jeancochrane Jan 7, 2019

Choose a reason for hiding this comment

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

Thanks for doing this research! I'll spend some time with your references to make sure that we're on the same page. If it's true that there's no way to get DROP TABLE and TRUNCATE to play nicely with nested transactions in MySQL, it'd probably be worth leaving a note on this in the docs.

'''

db_testdir.makepyfile("""
def test_drop_table(person, db_engine):
if db_engine.name == 'mysql':
return

# Drop the raw table
db_engine.execute('''
DROP TABLE "person"
DROP TABLE person
''')

# Check if the raw table exists
existing_tables = db_engine.execute('''
SELECT relname
FROM pg_catalog.pg_class
WHERE relkind in ('r', 'm')
AND relname = 'person'
''').first()
if db_engine.name == 'postgresql':
existing_tables = db_engine.execute('''
SELECT relname
FROM pg_catalog.pg_class
WHERE relkind in ('r', 'm')
AND relname = 'person'
''').first()

else:
raise ValueError(
'unsupported database engine type: ' + db_engine.name
)

assert not existing_tables

def test_drop_table_changes_dont_persist(person, db_engine):

existing_tables = db_engine.execute('''
SELECT relname
FROM pg_catalog.pg_class
WHERE relkind in ('r', 'm')
AND relname = 'person'
''').first()
if db_engine.name == 'mysql':
return

if db_engine.name == 'postgresql':
existing_tables = db_engine.execute('''
SELECT relname
FROM pg_catalog.pg_class
WHERE relkind in ('r', 'm')
AND relname = 'person'
''').first()

else:
raise ValueError(
'unsupported database engine type: ' + db_engine.name
)

assert existing_tables
""")
Expand Down