Add test coverage for PostgreSQL #28

juliotrigo · 2019-02-21T19:59:19Z

Travis CI:
- Use Ubuntu xenial
- Use Python 3.7
- Start PostgreSQL 9.6 service (PostgreSQL pre-installed versions: 9.4.20, 9.5.15, 9.6.11)
Install psycopg2 as part of the postgresql extras to run the tests
Run the tests also using PostgreSQL:
- Improve sorting tests: assert based on the fields being sorted
- Test that results are sorted only according to the provided filters.
- Do not test how rows with the same values are sorted since this is not consistent across RDBMS.
- Do not test whether NULL field values are placed first or last when sorting since this may differ across RDBMSs.
  - SQL defines that NULL values should be placed together when sorting, but it does not specify whether they should be placed first or last in the result.
  - (I'm currently preparing another PR to implement nullsfirst and nullslast).
Add Makefile targets to run MySQL and PostgreSQL Docker containers for testing.
Amend/improve README documentation:
- Fix inline literals
- Make line sizes consistent
- Mention all the RDBMS that are tested
- Improve the testing section
Fix CHANGELOG syntax.

juliotrigo · 2019-02-24T18:07:13Z

I'd be great if you could take a look when you have a moment @mattbennett @timbu @iky

mattbennett · 2019-02-24T19:57:19Z

.travis.yml

 services:
  - mysql

+addons:
+  postgresql: "9.6"


Might be easier to just use Docker here, since you're already providing docker Makefile targets for local dev. Then you're no longer restricted to whatever versions Travis has installed.

That sounds good 👍

mattbennett · 2019-02-24T19:58:42Z

README.rst

 Python 2
 --------

-There is no active support for python 2, however it is compatiable as of February 2019, if you install funcsigs.
+There is no active support for python 2, however it is compatiable as of


s/compatiable/compatible

mattbennett · 2019-02-24T20:00:38Z

test/conftest.py

@@ -69,9 +97,26 @@ def db_uri(request, config):


 @pytest.fixture(scope='session')
-def connection(db_uri):
+def is_postgresql(db_uri):
+    if POSTGRESQL_DB in db_uri:


It might actually be more confusing to have this defined as a constant than simply used inline. There is only one use, and it's not likely to change ;)

That's true. I'll change it.

mattbennett · 2019-02-24T20:07:05Z

test/interface/test_sorting.py

+    RDBMS sort rows with the same values being ordered since this is not
+    consistent across RDBMS.
+
+    Excludes `NULL` sorting from tests: sorting fields containing `NULL`


"Excludes nonexplicit sorting from tests" and "Excludes NULL sorting from tests" are confusing to me.

Do you mean just that this test does not verify these things because they differ across RDBMSs?

Yes, that's it.

I've rephrased it to (try to) make it readable. Basically:

Don't test how rows with the same values are sorted.

Don't test whether NULL field values are placed first or last when sorting.

Because these things differ across RDBMSs.

mattbennett · 2019-02-24T20:07:33Z

test/interface/test_sorting.py

+        result_same_name_1 = [
+            result[0].id, result[1].id, result[2].id, result[3].id
+        ]
+        result_same_name_4 = [result[5].id, result[6].id]


What's the significance of these variable names?

These variables contain a list of results that have the same order. This is to avoid duplicating the list of results multiple times in the test.

I have now renamed these variables to results_with_same_order_1 and results_with_same_order_2 since we have two groups of results with the same order in this test. This way, we also avoid including the field value (name_1, name_4) in the variable name, as it was done, even though _1 and _2 are not ideal.

Please let me know if you have any other suggestions.

juliotrigo · 2019-02-27T20:33:46Z

Thanks, @mattbennett for your review!

Comments addressed and ready for another look.

mattbennett · 2019-02-28T08:56:33Z

test/interface/test_sorting.py

+        results_with_same_order_1 = [
+            result[0].id, result[1].id, result[2].id, result[3].id
+        ]
+        results_with_same_order_2 = [result[5].id, result[6].id]


I still find this confusing, but now recognise that it is my fault for setting up the fixtures the way I did. Actually result_same_name_1 and result_same_name_4 were quite appropriate names!

Thinking about it a bit more though, why is the test making assertions about the IDs? Wouldn't it be sufficient to just assert that the names of the results are in ascending order? i.e.

results = sorted_query.all() assert [result.name for result in results] == ["name_1", "name_1", ..., "name_5"]

(Recognise this is my fault too -- I wrote the original test!)

Yes, that looks better! I've changed the tests as you suggested.

I have also s/result/results in the rest of the tests.

However, there are some other tests where we sort by multiple fields and it gets a bit more complicated to use this way of testing (as the number of filters grows). It seems easier to keep the assertions based on the IDs (since we just test the order of the rows), instead of adding all the field values to all the tests. On the other hand, by doing so you can also explicitely see how the fields are ordered:

assert len(results) == 8 assert results[0].id == 1 assert results[1].id == 3 assert results[2].id == 7 assert results[3].id == 5 assert results[4].id == 2 assert results[5].id == 6 assert results[6].id == 4 assert results[7].id == 8

vs.

assert [ (result.name, result.count, result.id) for result in results ] == [ ('name_1', 5, 1), ('name_1', 3, 3), ('name_1', 2, 7), ('name_1', 2, 5), ('name_2', 10, 2), ('name_4', 15, 6), ('name_4', 12, 4), ('name_5', 1, 8), ]

What do you think @mattbennett ? Would you apply the latter way of testing it to all the tests in that module?

I like the second way! All the tests consistently using this would be nice. Adding a simple comment saying "expect results to be ordered by name, then count" (or whatever) would make things very clear.

I've amended the assertions as you suggested @mattbennett and it looks way more readable!

It's easy now to compare the results with the actual filters placed a few lines above the assertions and visually check that it's sorted correctly. I think extra comments are not even needed at this point, having all the info that we need in the list comprehension (and the order_by dicts).

Please take another look when you have a moment.

Much nicer!

Use Ubuntu Xenial and Python 3.7

edcd854

juliotrigo added the WIP Work in progress label Feb 21, 2019

juliotrigo added 19 commits February 21, 2019 20:08

Add PostgreSQL v10 to Travis

43cd615

Remove Travis addons: GitHub check not going green

b759b11

Travis CI: use addons to install postgresql

cbcfa2a

Install psycopg2 as part of the tests

343080e

Run tests also using PostgreSQL

c2e5577

Only return DB engine options for PostgreSQL

c1c4de3

Exclude NULL sorting and non explicit sorting from tests

e1682c2

Add test fixture to know whether test are running on PostgreSQL

79e991b

Add Makefile targets to run RDMS docker containers for testing

29eac10

Improve documentation

4072fd0

Improve documentation

9ce72b4

Improve documentation

231d17c

Comments to keep DB versions synchronized

26f7081

Do not use Docker patch versions to be in sync with Travis CI

3387430

Update documentation

a365970

Improve documentation

69ac461

Fix README inline literals and make line sizes consistent

ee99669

Make README line sizes consistent

cb3a4aa

Amend docstring

7523dff

juliotrigo removed the WIP Work in progress label Feb 24, 2019

mattbennett reviewed Feb 24, 2019

View reviewed changes

juliotrigo added 6 commits February 27, 2019 18:30

Amend typo in README

462b102

Remove unnecessary constant

bd2c7a1

Use Docker containers for tests in Travis CI

a6bf6d5

Use Travis addons to update Docker via apt

aa3cf86

Travis CI apt update

7c97fe6

Remove Travis apt addons: it has no effect and doubles run time

cdffb25

juliotrigo added 3 commits February 27, 2019 19:43

Improve tests documentation

767f7dd

Rename variables in tests

56a034d

Fix CHANGELOG syntax

6af450c

mattbennett reviewed Feb 28, 2019

View reviewed changes

juliotrigo added 4 commits February 28, 2019 19:34

Improve test assertions and make variable name consistent

e45d260

Amend MySQL test container name

3c4bde4

Automatically clean up test docker containers

ce1ca02

Improve assertions in sorting tests

e9155eb

mattbennett approved these changes Mar 6, 2019

View reviewed changes

juliotrigo merged commit 12333bb into master Mar 6, 2019

juliotrigo deleted the support_postgresql branch March 6, 2019 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test coverage for PostgreSQL #28

Add test coverage for PostgreSQL #28

juliotrigo commented Feb 21, 2019 •

edited

Loading

juliotrigo commented Feb 24, 2019

mattbennett Feb 24, 2019

juliotrigo Feb 27, 2019

mattbennett Feb 24, 2019

mattbennett Feb 24, 2019

juliotrigo Feb 27, 2019

mattbennett Feb 24, 2019

juliotrigo Feb 27, 2019

mattbennett Feb 24, 2019

juliotrigo Feb 27, 2019

juliotrigo commented Feb 27, 2019

mattbennett Feb 28, 2019

mattbennett Feb 28, 2019

juliotrigo Feb 28, 2019

mattbennett Mar 2, 2019

juliotrigo Mar 5, 2019

mattbennett Mar 6, 2019

Add test coverage for PostgreSQL #28

Add test coverage for PostgreSQL #28

Conversation

juliotrigo commented Feb 21, 2019 • edited Loading

juliotrigo commented Feb 24, 2019

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

juliotrigo commented Feb 27, 2019

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

juliotrigo commented Feb 21, 2019 •

edited

Loading