Fix postgres tests #150
Fix postgres tests #150
Conversation
…hen using postgres. Unfortunetly SQLLite is a lot less strict with types compared to most production RD's and does not throw errors when incorrect types are used in query parameter (or even when incorrect types are inserted afaik). Incorrect types are being used because the common tests have been created more with Mongo DB in mind, and queries involving non-existent ID's use mongo conventions (e.g they don't use integers for id's). While this change works, a potential optimization is to do type checking and throw early errors when incorrect id types are used.
|
Awesome! This looks like it would do it but it does not run against Postgres on CI. Could you branch off #108 and apply your changes to that in a new PR? |
| @@ -83,7 +83,7 @@ class Service { | |||
| return result[0]; | |||
| }) | |||
| .then(select(params, this.id)) | |||
| .catch(utils.errorHandler); | |||
| .catch(() => { throw new errors.NotFound(`No record found for id '${id}'`); }); | |||
There was a problem hiding this comment.
This could happen for other errors as well though right?
There was a problem hiding this comment.
That is correct. Since we are doing this in the get, in most cases it should be the correct behaviour.
Again it might comes down to how you want to handle types. If a query parameter uses the incorrect type should an error be thrown? Or should there just be a 'Not Found' error? This catch still might be too broad regardless though - like what if the DB is not available.
In any case my thoughts were that there are probably better ways to do this, but since the tests are passing and it is quite important to get the tests running against a 'proper' RDB it is probably a good idea to just run with this for now. (From my side it seems quite problematic using SQLLite for tests because it behaves differently to most other RDB's and in addition the code has some branches for specific dialects)
|
I will re-branch of #108 |
|
@daffl I am not familiar with the CI tooling but it looks like it is already running against Postgres? I actually branched off your postgres branch btw. |
|
Are you sure? It should show the diff from https://github.com/feathersjs/feathers-sequelize/pull/108/files in here then as well. I can merge your branch into that one as well if that makes it easier for you. |
|
Well that is a diff between your postgres branch and master, this is a pull request from my postgres fixes branch into your postgres branch (not master). So it wont show that diff. :-) |
|
And here is the output from the CI tests: https://travis-ci.org/feathersjs/feathers-sequelize/jobs/265682341#L1208 |
|
Oh sorry, you're pulling into that branch as well. Great! I'll see if there is anything I need to do for my comment and then we can release it. Thanks a lot for doing this! |
Fixed all the broken postgres tests.