Skip to content
This repository has been archived by the owner on Sep 18, 2018. It is now read-only.

Fix/postgres fixture #185

Merged
merged 3 commits into from
Sep 10, 2016
Merged

Conversation

waltherg
Copy link
Contributor

@waltherg waltherg commented Sep 9, 2016

This PR fixes the following issues:

  • The drop database statement had no effect due to a missing semicolon

@coveralls
Copy link

coveralls commented Sep 9, 2016

Coverage Status

Coverage increased (+0.08%) to 63.76% when pulling 492e2c8 on markovianhq:fix/postgres_fixture into 441bf09 on ClearcodeHQ:master.

@coveralls
Copy link

coveralls commented Sep 9, 2016

Coverage Status

Coverage increased (+0.04%) to 63.72% when pulling 492e2c8 on markovianhq:fix/postgres_fixture into 441bf09 on ClearcodeHQ:master.

@@ -121,7 +121,7 @@ def drop_postgresql_database(psycopg2, user, host, port, db):
conn = psycopg2.connect(user=user, host=host, port=port)
conn.set_isolation_level(psycopg2.extensions.ISOLATION_LEVEL_AUTOCOMMIT)
cur = conn.cursor()
cur.execute('DROP DATABASE IF EXISTS %s' % db)
cur.execute('DROP DATABASE IF EXISTS %s;' % db)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why other queries work without that semicolon, but this one doesn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's an example of a query that works without terminating semicolon?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have cur.execute('CREATE DATABASE ' + db). Also the DROP DATABASE works (I do get errors from it if a buggy test leaves an open database connection). I cannot find explicit rules about semicolons, but it seems the single statement passed via cur.execute doesn't need a trailing one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. The postgres fixture is unusable for me without the semicolon
since the db isn't cleared. You're saying the fixture works for you without
the semicolon?
On 10 Sep 2016 12:11, "Michał Masłowski" notifications@github.com wrote:

In src/pytest_dbfixtures/factories/postgresql.py
#185 (comment)
:

@@ -121,7 +121,7 @@ def drop_postgresql_database(psycopg2, user, host, port, db):
conn = psycopg2.connect(user=user, host=host, port=port)
conn.set_isolation_level(psycopg2.extensions.ISOLATION_LEVEL_AUTOCOMMIT)
cur = conn.cursor()

  • cur.execute('DROP DATABASE IF EXISTS %s' % db)
  • cur.execute('DROP DATABASE IF EXISTS %s;' % db)

We have cur.execute('CREATE DATABASE ' + db). Also the DROP DATABASE
works (I do get errors from it if a buggy test leaves an open database
connection). I cannot find explicit rules about semicolons, but it seems
the single statement passed via cur.execute doesn't need a trailing one.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/ClearcodeHQ/pytest-dbfixtures/pull/185/files/492e2c8e62eac3b385abc095bd46bf2d08c93f9d#r78275121,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADHzHpb3TlFNnWr09vJuzjxfvzUGNtzXks5qooJggaJpZM4J5j-u
.

Copy link
Member

Choose a reason for hiding this comment

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

yes, we haven't had any problems with this except for errors we get when some process used in integration tests doesn't close the connection at the end of the test.

I'm still going to accept this for the sake of correspondence with other tools (better to add comma after SQL statement here, than forget it about in other places.

Still even documentation doesn't always use the semicolon for single statement execute: http://initd.org/psycopg/docs/usage.html

It could help to get to the issue, if you'd write something about your test setup or test case.

@fizyk fizyk merged commit 492e2c8 into ClearcodeHQ:master Sep 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants