Adding MySQL support for db fixtures #10

kamikaz1k · 2019-01-04T03:14:40Z

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)

Also updated dependencies and tests for fixtures

jeancochrane · 2019-01-04T13:48:06Z

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

jeancochrane

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

jeancochrane · 2019-01-05T22:22:18Z

tests/_conftest.py

-    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)


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.

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.

jeancochrane · 2019-01-05T23:00:23Z

tests/_conftest.py

@@ -47,6 +61,7 @@ def app(database):
    app = Flask(__name__)

    app.config['SQLALCHEMY_DATABASE_URI'] = DB_CONN
+    app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = True


What's your reasoning behind adding this config?

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.

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

jeancochrane · 2019-01-05T23:00:45Z

pytest_flask_sqlalchemy/fixtures.py

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


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

jeancochrane · 2019-01-05T23:01:45Z

.travis.yml

+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"


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

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.

jeancochrane · 2019-01-05T23:08:54Z

tests/test_fixtures.py

+    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


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.

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;

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.

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 · 2019-02-20T22:22:28Z

Hey @kamikaz1k! Any progress on this recently?

kamikaz1k · 2019-02-23T19:20:27Z

@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 · 2019-02-26T00:54:46Z

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.

pmdarrow · 2019-03-20T17:40:28Z

tests/_conftest.py

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


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 · 2019-09-30T04:42:15Z

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 :)

kamikaz1k added 6 commits December 5, 2018 23:52

Attempting multi db testing

0adba51

only one python to test multi db feature

e179192

just trying to make pg work

aeb15c8

added mysql

90a37c9

Updated db fixtures to also work with MySQL

cbe1306

Also updated dependencies and tests for fixtures

re-added python 3.4 and 3.5 to travis

db4de7a

jeancochrane reviewed Jan 5, 2019

View reviewed changes

Switched from pytest-mysql to bare PyMySQL connector

7608cf4

jeancochrane mentioned this pull request Feb 20, 2019

Is it possible to use this with FactoryBoy? #12

Open

pmdarrow reviewed Mar 20, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding MySQL support for db fixtures #10

Adding MySQL support for db fixtures #10

kamikaz1k commented Jan 4, 2019

jeancochrane commented Jan 4, 2019

jeancochrane left a comment

jeancochrane Jan 5, 2019

kamikaz1k Jan 6, 2019

jeancochrane Jan 5, 2019

kamikaz1k Jan 6, 2019

jeancochrane Jan 7, 2019

jeancochrane Jan 5, 2019

jeancochrane Jan 5, 2019

kamikaz1k Jan 6, 2019

jeancochrane Jan 5, 2019

kamikaz1k Jan 6, 2019 •

edited

Loading

kamikaz1k Jan 6, 2019

jeancochrane Jan 7, 2019 •

edited

Loading

jeancochrane commented Feb 20, 2019

kamikaz1k commented Feb 23, 2019

jeancochrane commented Feb 26, 2019

pmdarrow Mar 20, 2019 •

edited

Loading

kamikaz1k commented Sep 30, 2019

	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.

Adding MySQL support for db fixtures #10

Are you sure you want to change the base?

Adding MySQL support for db fixtures #10

Conversation

kamikaz1k commented Jan 4, 2019

jeancochrane commented Jan 4, 2019

jeancochrane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamikaz1k Jan 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

jeancochrane commented Feb 20, 2019

kamikaz1k commented Feb 23, 2019

jeancochrane commented Feb 26, 2019

pmdarrow Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

kamikaz1k commented Sep 30, 2019

kamikaz1k Jan 6, 2019 •

edited

Loading

jeancochrane Jan 7, 2019 •

edited

Loading

pmdarrow Mar 20, 2019 •

edited

Loading