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

Conversation

kamikaz1k
Copy link

Adding MySQL (tested on 5.7) support with updated tests. The travis config was updated to support testing on multiple databases. I will try to tackle SQLite next.

NOTE: I skip the drop table tests for MySQL engines because the DROP TABLE ... statement causes an autocommit, which breaks what the test was trying to assert. After scratching my head quite a lot I found these in the MySQL docs: https://dev.mysql.com/doc/refman/5.7/en/implicit-commit.html

"[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."

If anybody thinks I have misunderstood, I'll gladly take the lesson!

Refs #3 (kinda)

@jeancochrane
Copy link
Owner

Thanks for opening this @kamikaz1k! I'll have some time to give it a review on Saturday.

Copy link
Owner

@jeancochrane jeancochrane left a comment

Choose a reason for hiding this comment

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

This is a great start! There are a few outstanding MySQL-related questions that still need some clarification.

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.

@@ -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.

@@ -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.

.travis.yml Outdated
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.

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

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.

@jeancochrane
Copy link
Owner

Hey @kamikaz1k! Any progress on this recently?

@kamikaz1k
Copy link
Author

@jeancochrane sorry, I might have misunderstood. I thought you were going to confirm my findings about MySQL behaviour for Table/Transactions. Or did you just mean I should make updates to the README?

@jeancochrane
Copy link
Owner

Sorry @kamikaz1k, I lost the context for this PR and forgot that the ball was in my court. I'll have some time to take a closer look soon.


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.

@kamikaz1k
Copy link
Author

Hey @jeancochrane, just wondering if you made a decision around this PR/MySQL support.

My company just started switching over our applications to use pytest, and I was hoping to use this addon in our new setup :)

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

3 participants