Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
User Management backend - Base - DB migrations #46
base: master
Are you sure you want to change the base?
User Management backend - Base - DB migrations #46
Changes from 2 commits
bc0d2a1
cbba2fe
0b13b2e
0160a87
f9bac1f
d86a25e
6bc32bb
3dcd5dc
429ab2c
a3dcc6d
7b17dcd
6a9ad8a
cb7a83b
a59b9c0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you don't want to save this data because its a test. If you did, you would need to call commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, probably the reason it didn't reset your sequence is because you didn't commit the transaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Any other tests are really redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that
cursor.execute
is enough because when I ran the testdef 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want it to be called after the cursor will finish his job every test, therefore I put
db_connection.rollback()
right after theyield cursor
statement. But I have to be honest I don't understand theyield
statements into their depth...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I call rollback after the cursor finish his job so the data still in the DB in this point. This test was passed.
This file was deleted.