User Management backend - Base - DB migrations #46

ahinoamta · 2020-05-19T06:23:30Z

fixes #27.
This PR depends on PR #38.
Added Flask-Migrate extension for database migrations support.
The content inside edison/migrations folder is auto-created by the extension.

@DoRTaL94 @RoniRush

Added Flask-Migrate extension for database migrations support. The content inside edison/migrations folder is auto-created by the extension.

…db-migrations

Added Flask-Migrate extension for database migrations support. The content inside edison/migrations folder is auto-created by the extension.

…to db-migrations # Conflicts: # requirements.txt

ahinoamta · 2020-05-19T17:20:11Z

tests/conftest.py

+    ("ahinoam",))
+
+@pytest.fixture
+def reset_users_primary_key(cursor):


Hey @ifireball 😄
I wrote a few tests for checking the DB like you asked me in the last PR.
Can you please help me to find a way to reset the primary key sequence after the tests will be finished?
I tried to enter this fixture in the 'cursor' fixture (right after the 'yield'), but it didn't work, I assume it happened because after the 'yield' the cursor is not available anymore?

why would you want to reset the primary key sequence? It should have no special meaning, so it won't matter if the first user is 1 or 140

In any case, probably the reason it didn't reset your sequence is because you didn't commit the transaction.

One last comment. The tests that you wrote here verify basic database functionality, which doesn't need to be tested.
What you should probably test is:

the database exists

you can connect to it

the tables you created exist.

Any other tests are really redundant.

I want to reset the primary key in order to clean up my database after the tests and return it to the initial state. I think it will make more sense when the database show you data that begins with id 1.

About the commit after the execute, I used the “cursor.execute” without commit after it but still the query was executed. So why in this case I should add the commit?
Furthermore, I don’t know where to call this fixture, because if I inject it to “cursor” it will activate it in the beginning of the “cursor” fixture, no?

Ack

1 - the alter sequence statement is correct and it should work. Try to remember, in the lifetime of an app the database sequence will very rarely stay sequential.
2 - Have you tried executing this code and then going into the database using psql and seeing if the data you added was still there? As far as i know pyscopg does not have automated commits. One possibly confusing thing about sequences and transactions is when you get a new sequence that ignores the transaction, so if you insert and then rollback the transaction, the sequence is still at the next number.
On line 20 you have db_connection.rollback() when is this code called? A rollback means erase everything that I've done during this current transaction.

Have you tried executing this code and then going into the database using psql and seeing if the data you added was still there?

I thought that cursor.execute is enough because when I ran the test def test_username(cursor, build_users_table) it was passed, so the row 'ahinoam' did enter to the DB.
But when I checked it in the DB itself with psql, it didn't appear there (and it's not because db_connection.rollback() because I put it in a comment).
Do you have any idea what is the reason for that?
Maybe it's because I remain in the same transaction?

On line 20 you have db_connection.rollback() when is this code called? A rollback means erase everything that I've done during this current transaction.

I want it to be called after the cursor will finish his job every test, therefore I put db_connection.rollback() right after the yield cursor statement. But I have to be honest I don't understand the yield statements into their depth...

The way the database works is that during the same transaction everything looks like it is saved, but it really isn't until you call commit.
If you don't manually call commit, it is the same as calling rollback.

simzacks · 2020-05-20T05:42:31Z

tests/conftest.py

+    ("ahinoam",))
+
+@pytest.fixture
+def reset_users_primary_key(cursor):


why would you want to reset the primary key sequence? It should have no special meaning, so it won't matter if the first user is 1 or 140

simzacks · 2020-05-20T05:46:15Z

tests/conftest.py

+    ("ahinoam",))
+
+@pytest.fixture
+def reset_users_primary_key(cursor):


In any case, probably the reason it didn't reset your sequence is because you didn't commit the transaction.

simzacks · 2020-05-20T05:49:53Z

tests/conftest.py

+    cursor.execute("""
+    INSERT INTO users (username) VALUES (%s);
+    """,
+    ("ahinoam",))


I assume you don't want to save this data because its a test. If you did, you would need to call commit

simzacks · 2020-05-20T05:52:09Z

tests/conftest.py

@@ -0,0 +1,33 @@
+import pytest
+import psycopg2


You might want to use sqlalchemy to test that the database was created properly, so that your tests are uniform with your code. Either way works the same, one difference is if you switch database engines (one of the benefits of using an ORM) you would have to rewrite this code.

.gitignore

simzacks · 2020-05-20T06:05:23Z

tests/db_test.py

+import psycopg2
+
+
+def test_users_table_rows_count(cursor, build_users_table):


Where is this cursor defined? The only place I see it is in conftest.py and this script doesn't import that one.
It also might make sense to have one database test file and not split it up.

Also, if I understood the code in the conftest.py correctly, you always call rollback, which erases all of your changes. So if you are expecting the data to be here because of that execution, it might not be.

Where is this cursor defined? The only place I see it is in conftest.py and this script doesn't import that one.
It also might make sense to have one database test file and not split it up.

The cursor is defined in conftest.py file that aggregate all the fixtures in one place. I tried to keep the db_test.py file short.

Also, if I understood the code in the conftest.py correctly, you always call rollback, which erases all of your changes. So if you are expecting the data to be here because of that execution, it might not be.

I call rollback after the cursor finish his job so the data still in the DB in this point. This test was passed.

simzacks · 2020-05-20T06:09:36Z

tests/conftest.py

+    ("ahinoam",))
+
+@pytest.fixture
+def reset_users_primary_key(cursor):


One last comment. The tests that you wrote here verify basic database functionality, which doesn't need to be tested.
What you should probably test is:

the database exists

you can connect to it

the tables you created exist.

Any other tests are really redundant.

ahinoamta · 2020-05-22T09:13:01Z

tests/db_test.py

+    assert database_exists(db_uri) is True
+
+def test_db_connection(engine):
+    assert engine.connect() is not None


@simzacks I'm not sure with this db connection check, I tried to find something more correct but without success

@ahinoamta what isn't working with this code?
IIUC, you are:

connecting to the database, which verifies that it exists in the location/port that you are expecting it to be

verifying that the expected tables exist.

This code is working, but don't you think the check needs to be more specific and not just is not None?

2 questions

Is there ever a time that you call database_exists and it returns true, but engine.connect would fail?

If you could not connect to the engine, what would engine.connect return?

@simzacks

I think there is no such case, I googled it and I didn't find any other case that can cause failure to engine.connect. I did find that the engine provides the Connection from the connection pool but I'm not sure it can cause connection failures (assuming the DB is up).

If the database doesn't exist, so I won't be able to connect to the engine, it will throw an exception- 'sqlalchemy.exc.OperationalError'.

simzacks · 2020-05-22T12:45:34Z

tests/db_test.py

+
+def test_db_tables_exist(engine):
+    tables = sqlalchemy.inspect(engine).get_table_names()
+    assert tables == ['users']


Assumedly, tables will return a list. you might want to try:
assert "users" in tables (if there are more tables you want to test) and "..." in tables

tests/db_test.py

DoRTaL94 and others added 8 commits April 25, 2020 19:18

Added Flask-Migrate extension

bc0d2a1

Added Flask-Migrate extension for database migrations support. The content inside edison/migrations folder is auto-created by the extension.

Merge branch 'db-support' of https://github.com/DoRTaL94/edison into …

cbba2fe

…db-migrations

Merge branch 'db-support' of https://github.com/DoRTaL94/edison into …

0b13b2e

…db-migrations

Added Flask-Migrate extension

0160a87

Added Flask-Migrate extension for database migrations support. The content inside edison/migrations folder is auto-created by the extension.

Deleted DB restore process

f9bac1f

Added pytest to requirements.txt

d86a25e

Merge branch 'db-migrations' of https://github.com/DoRTaL94/edison in…

6bc32bb

…to db-migrations # Conflicts: # requirements.txt

Updated .gitignore

3dcd5dc

ahinoamta assigned ahinoamta, DoRTaL94 and RoniRush May 19, 2020

ahinoamta marked this pull request as draft May 19, 2020 09:00

ahinoamta added 2 commits May 19, 2020 20:07

Updated .gitignore

429ab2c

Added few tests for db connection

a3dcc6d

ahinoamta commented May 19, 2020

View reviewed changes

ahinoamta marked this pull request as ready for review May 19, 2020 17:21

simzacks requested changes May 20, 2020

View reviewed changes

simzacks mentioned this pull request May 20, 2020

User Management backend - Base - DB Models #47

Merged

CR fixes- updated db_test.py

7b17dcd

ahinoamta commented May 22, 2020

View reviewed changes

ahinoamta requested a review from simzacks May 22, 2020 09:14

simzacks reviewed May 22, 2020

View reviewed changes

CR fixes

6a9ad8a

DoRTaL94 reviewed May 30, 2020

View reviewed changes

tests/db_test.py Show resolved Hide resolved

DoRTaL94 requested a review from simzacks May 30, 2020 11:10

Added scope to pytest fixture

cb7a83b

ahinoamta requested a review from ifireball May 31, 2020 20:36

CR fix

a59b9c0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User Management backend - Base - DB migrations #46

User Management backend - Base - DB migrations #46

ahinoamta commented May 19, 2020

ahinoamta May 19, 2020 •

edited

Loading

simzacks May 20, 2020

simzacks May 20, 2020

simzacks May 20, 2020

ahinoamta May 21, 2020

simzacks May 21, 2020

ahinoamta May 22, 2020 •

edited

Loading

ahinoamta May 22, 2020

simzacks May 22, 2020

simzacks May 20, 2020

simzacks May 20, 2020

simzacks May 20, 2020

simzacks May 20, 2020

simzacks May 20, 2020

simzacks May 20, 2020

ahinoamta May 22, 2020

ahinoamta May 22, 2020

simzacks May 20, 2020

ahinoamta May 22, 2020

simzacks May 22, 2020

ahinoamta May 24, 2020

simzacks May 24, 2020

ahinoamta May 25, 2020

simzacks May 22, 2020

ahinoamta May 24, 2020

		import psycopg2


		def test_users_table_rows_count(cursor, build_users_table):

User Management backend - Base - DB migrations #46

Are you sure you want to change the base?

User Management backend - Base - DB migrations #46

Conversation

ahinoamta commented May 19, 2020

ahinoamta May 19, 2020 • edited Loading

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

ahinoamta May 22, 2020 • edited Loading

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

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

ahinoamta May 19, 2020 •

edited

Loading

ahinoamta May 22, 2020 •

edited

Loading