Add tests against postgres, mysql and sqlite and fixes for #21 #28 #27
Conversation
|
I've updated PR with fix to issue with NotNullViolationError |
|
Updated PR with fix for #28 |
|
Well updated PR with offset implementation #8 |
|
@kiddick I think it's great that you found fixes for the various issues you mentioned, but I believe it's better practice to try and submit a different PR for each one, to make the reviewing process easier. It also makes it easier for others to see if there's already a PR for what they'd like to work on. #38 is redundant with this PR, most likely because @thiagovarela didn't see that this PR fixed #21 too (but one can't tell from the title of this PR, which says it just adds tests). Same for #30 and #32. I think it might be a good idea to remove the last commits (those about |
|
@florimondmanca well that makes sense |
kiddten
changed the title
| before_script: | ||
| - psql -c 'create database test_database;' -U postgres | ||
| - echo 'create database test_database;' | mysql | ||
|
|
||
| script: | ||
| - scripts/test |
There was a problem hiding this comment.
I wonder if we could do something like
script:
- DATABASE_URL=postgresql://localhost/test_database scripts/test
- DATABASE_URL=mysql://localhost/test_database scripts/test
- DATABASE_URL=sqlite:///test.db scripts/testand then avoid looping through databases.
There was a problem hiding this comment.
Yes, I agree that this would be a cleaner way to do it.
I'm also concerned that (at first glance at least) there seems to be a lot of code duplication here (either associated with database looping or not).
There was a problem hiding this comment.
actually I took same approach as https://github.com/encode/databases/blob/master/.travis.yml#L12
There was a problem hiding this comment.
@kiddick Yes, I think actually the issue is not the Travis config file but really the duplicated looping. It seems the databases repo uses pytest.mark.parametrize to make that cleaner — see here.
| required = [key for key, value in fields.items() if not value.has_default()] | ||
| validator = typesystem.Object( | ||
| properties=fields, required=required, additional_properties=False | ||
| ) | ||
| kwargs = validator.validate(kwargs) | ||
| # remove pk with NULL value to avoid constraint violation in postgres | ||
| if pk.allow_null: | ||
| del kwargs[pkname] |
There was a problem hiding this comment.
@kiddick
I think this will ignore the pk value provided by client.
for example:
class User(orm.Model):
__tablename__ = "users"
id = orm.Integer(primary_key=True)
name = orm.String(max_length=100)
user = await User.objects.create(id=100000001, name="Tom")
assert user.id == 100000001
There was a problem hiding this comment.
I add a test for this case in kiddten#1
|
I guess this is not needed anymore. Thanks for the effort though. |
Related to #21, #28