-
Notifications
You must be signed in to change notification settings - Fork 74
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
Add test coverage for PostgreSQL #28
Conversation
I'd be great if you could take a look when you have a moment @mattbennett @timbu @iky |
.travis.yml
Outdated
services: | ||
- mysql | ||
|
||
addons: | ||
postgresql: "9.6" |
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.
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.
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.
That sounds good 👍
README.rst
Outdated
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 |
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.
s/compatiable/compatible
test/conftest.py
Outdated
@@ -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: |
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.
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 ;)
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.
That's true. I'll change it.
test/interface/test_sorting.py
Outdated
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` |
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.
"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?
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.
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.
test/interface/test_sorting.py
Outdated
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] |
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.
What's the significance of these variable names?
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.
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.
Thanks, @mattbennett for your review! Comments addressed and ready for another look. |
test/interface/test_sorting.py
Outdated
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] |
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 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"]
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.
(Recognise this is my fault too -- I wrote the original test!)
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.
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?
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 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.
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'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.
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.
Much nicer!
xenial
9.6
service (PostgreSQL pre-installed versions:9.4.20
,9.5.15
,9.6.11
)psycopg2
as part of thepostgresql
extras to run the testsNULL
field values are placed first or last when sorting since this may differ across RDBMSs.NULL
values should be placed together when sorting, but it does not specify whether they should be placed first or last in the result.nullsfirst
andnullslast
).