Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
8 changes: 7 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,16 @@ 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' = 'mysql' ]; then mysql -e 'DROP DATABASE IF EXISTS pytest_test;'; fi"

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', 'mysqlclient']},
classifiers=[
'Development Status :: 4 - Beta',
'Environment :: Plugins',
Expand Down
52 changes: 43 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
import MySQLdb
from pytest_postgresql.factories import (init_postgresql_database,
drop_postgresql_database)

Expand All @@ -17,26 +18,59 @@
'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']


def _init_mysql_database(request, db_user, db_host, db_name, db_pass=''):

mysql_conn = MySQLdb.connect(
host=db_host,
user=db_user,
passwd=db_pass
)

mysql_conn.query("CREATE DATABASE IF NOT EXISTS {name}".format(name=db_name))

mysql_conn.query("USE {name}".format(name=db_name))

@request.addfinalizer
def drop_database():
mysql_conn.query("DROP DATABASE IF EXISTS {name}".format(name=db_name))
mysql_conn.close()

return mysql_conn


@pytest.fixture(scope='session')
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':
_init_mysql_database(request, db_user, db_host, db_name)

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 Down
53 changes: 38 additions & 15 deletions tests/test_fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ def test_db_session_changes_dont_persist(person, db_engine, db_session):
assert not db_session.query(person).first()
""")

# Run tests
result = db_testdir.runpytest()
result.assert_outcomes(passed=2)

Expand Down Expand Up @@ -212,33 +211,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