Add support for running tests with Postgres #299

dtao · 2018-07-14T20:43:56Z

This adds support for setting the DATABASE_URL environment variable to run tests with database engines other than sqlite3 locally. It also configures the CI build to run the tests with Postgres.

dtao · 2018-07-14T20:46:03Z

Note that this is actually a precursor to the fix for #296. As discussed on that ticket, the test for the fix involves threads, which don't work with sqlite3; so this sets up the Travis build to run the tests with Postgres.

I am doing this as its own PR because these changes are independent of the fix for #296, and it didn't feel right to me to lump them all together in a single pull request.

dtao · 2018-07-14T20:49:26Z

.travis.yml

- include:
-  - python: 3.6
-    env: TOXENV=i18n
+  include:


The indentation in this file was inconsistent. The Python versions were indented with two spaces, but everything else was indented with one space.

I opted to indent everything two spaces. Happy to revert these particular changes if they're objectionable for some reason.

dtao · 2018-07-14T20:50:50Z

test_settings.py

+    except ImportError:
+        print('Using the DATABASE_URL variable requires dj-database-url and '
+              'psycopg2. Try:\n\npip install -r travis.txt')
+        sys.exit(1)


My feeling is that it's better to error and exit in this scenario rather than falling back to a database backend that isn't what was specified for the environment.

You could also either (a) let the exception bubble up or (b) re-raise after printing the error message. Either way, an exception at this point will halt execution.

Agreed, better not to call sys.exit() and let it exit on its own by re-raising.

I don't love the implicit dependency on psycopg2 just because someone has defined DATABASE_URL at all—it might be useful to run the tests against MariaDB or to use PyGreSQL. But I don't think it's worth blocking this on.

I agree re: the dependency on psycopg2. I was taking a "Don't build it until you need it" approach, figuring that the next person who comes along and wants to run tests locally with a different database could submit their own pull request to make the logic to handle the ImportError more surgical. Perhaps that was selfish of me.

I'm happy to make the logic here a little more conservative, only requiring psycopg2 if the DB backend is actually Postgres.

Nah, I think your approach is right, we can add if we need later

dtao · 2018-07-14T20:52:21Z

tox.ini

@@ -8,6 +8,7 @@ deps =
    django111: Django>=1.11,<2.0
    django20: Django>=2.0,<2.1
    -rtravis.txt
+passenv = DATABASE_URL


By default, tox only passes the PATH environment variable. This is necessary to get the value for DATABASE_URL defined in .travis.yml passed to tox.

dtao · 2018-07-14T20:53:03Z

travis.txt

@@ -2,6 +2,8 @@
 # versions.
 mock==1.3.0
 Jinja2>=2.7.1
+dj-database-url==0.5.0


This just provides a convenient way of parsing the DATABASE_URL environment variable. I felt it was fairly harmless to add a dependency here, since these are after all only the CI requirements.

I feel like I've implemented this myself with urlparse so many times. TIL!

dtao · 2018-07-14T20:53:25Z

travis.txt

 django-jinja>=2.1,<3
+psycopg2>=2.7.5


This is obviously to allow Postgres to be used as the DB engine.

dtao · 2018-07-14T21:04:19Z

The CI build is failing, but I'm not sure why. As far as I know I followed the instructions for setting up PostgreSQL in Travis correctly. The docs say:

The default user for accessing the local PostgreSQL server is postgres with a blank password.

Yet the test failure seems to indicate:

django.db.utils.OperationalError: could not translate host name "postgres" to address: Name or service not known

So I don't know. I'm going to try explicitly setting the port number, but if that doesn't work I'm out of ideas. Maybe somebody else knows what magic incantation is required to get it working.

dtao · 2018-07-14T21:41:15Z

Ah, I see my mistake now. I misread: the docs only say that the default user is postgres; it seems the hostname to use to connect to the database server is just localhost.

Now the tests are passing :)

clintonb

Add a comment to .travis.yml and/or the README to inform other developers that Travis tests use PostgreSQL, and explain why.

@jsocol any feedback? If not, I will merge in ~72 hours.

clintonb · 2018-07-15T04:44:55Z

test_settings.py

+    except ImportError:
+        print('Using the DATABASE_URL variable requires dj-database-url and '
+              'psycopg2. Try:\n\npip install -r travis.txt')
+        sys.exit(1)


You could also either (a) let the exception bubble up or (b) re-raise after printing the error message. Either way, an exception at this point will halt execution.

jsocol

@clintonb Agree about sys.exit(), one other small but non-blocking comment. Thanks @dtao!

jsocol · 2018-07-15T14:08:15Z

test_settings.py

+    except ImportError:
+        print('Using the DATABASE_URL variable requires dj-database-url and '
+              'psycopg2. Try:\n\npip install -r travis.txt')
+        sys.exit(1)


Agreed, better not to call sys.exit() and let it exit on its own by re-raising.

I don't love the implicit dependency on psycopg2 just because someone has defined DATABASE_URL at all—it might be useful to run the tests against MariaDB or to use PyGreSQL. But I don't think it's worth blocking this on.

jsocol · 2018-07-15T14:09:32Z

travis.txt

@@ -2,6 +2,8 @@
 # versions.
 mock==1.3.0
 Jinja2>=2.7.1
+dj-database-url==0.5.0


I feel like I've implemented this myself with urlparse so many times. TIL!

This adds support for setting the DATABASE_URL environment variable to run tests with database engines other than sqlite3 locally. It also configures the CI build to run the tests with Postgres.

dtao · 2018-07-15T20:41:21Z

OK, I've added a comment to .travis.yml as @clintonb suggested and changed the logic in test_settings.py to raise an ImportError rather than sys.exit(1).

I squashed these commits because it's my understanding that is preferred for this library.

dtao force-pushed the use-postgres-in-ci-builds branch from a20c895 to 08b304c Compare July 14, 2018 20:47

dtao commented Jul 14, 2018

View reviewed changes

dtao force-pushed the use-postgres-in-ci-builds branch 2 times, most recently from 972ba5a to 1be78f4 Compare July 14, 2018 21:38

dtao mentioned this pull request Jul 14, 2018

Flush cache after the current transaction is committed (#296) #300

Merged

clintonb approved these changes Jul 15, 2018

View reviewed changes

jsocol reviewed Jul 15, 2018

View reviewed changes

Add support for running tests with Postgres

1617605

This adds support for setting the DATABASE_URL environment variable to run tests with database engines other than sqlite3 locally. It also configures the CI build to run the tests with Postgres.

dtao force-pushed the use-postgres-in-ci-builds branch from 1be78f4 to 1617605 Compare July 15, 2018 20:39

clintonb approved these changes Jul 16, 2018

View reviewed changes

clintonb merged commit 07b0736 into jazzband:master Jul 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for running tests with Postgres #299

Add support for running tests with Postgres #299

dtao commented Jul 14, 2018

dtao commented Jul 14, 2018

dtao Jul 14, 2018

dtao Jul 14, 2018

clintonb Jul 15, 2018

jsocol Jul 15, 2018

dtao Jul 15, 2018

jsocol Jul 15, 2018

dtao Jul 14, 2018

dtao Jul 14, 2018

jsocol Jul 15, 2018

dtao Jul 14, 2018

dtao commented Jul 14, 2018

dtao commented Jul 14, 2018

clintonb left a comment

clintonb Jul 15, 2018

jsocol left a comment

jsocol Jul 15, 2018

jsocol Jul 15, 2018

dtao commented Jul 15, 2018

		django-jinja>=2.1,<3
		psycopg2>=2.7.5

Add support for running tests with Postgres #299

Add support for running tests with Postgres #299

Conversation

dtao commented Jul 14, 2018

dtao commented Jul 14, 2018

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

dtao commented Jul 14, 2018

dtao commented Jul 14, 2018

clintonb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsocol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtao commented Jul 15, 2018