Update feathers-service-tests to version 0.8.1 🚀 #124
Conversation
|
This is actually a bit of a disaster. There are a lot of changes that need to happen to the mongoose test suite because of how we are adding |
|
That's why I didn't get to it yet. Maybe the populate and nested collections should be tested separate to the standard service tests. |
|
I agree they should be, unless we start to move that stuff to hooks (which is already happening). I'm also wondering how much time to sink into this module if we are contemplating deprecating it in favour of our own schema def + feathers-mongodb. |
|
It's the most downloaded database module so I don't think we can just drop it. I'd rather deprecate Levelup and Waterline first. I'll look into fixing this. |
daffl
force-pushed
the
greenkeeper-feathers-service-tests-0.8.1
branch
from
9377ee2 to
40f3b35
Compare
|
This should be fixed now. |
| const query = params.query || {}; | ||
| const patchQuery = {}; | ||
|
|
||
| // Account for potentially modified data |
There was a problem hiding this comment.
@daffl Can you elaborate or add a more detailed comment on what this is actually doing? I know it fixes something but I don't really understand what this is for. Is there a test that this refers to?
There was a problem hiding this comment.
The test is https://github.com/feathersjs/feathers-service-tests/blob/master/src/common-tests.js#L525. Basically if you did something like .patch(null, { name: 'Eric' }, { query: { name: 'David' } }) you didn't get events or the changed items. This is also not a complete fix but covers a fairly common case.There does not seem to be a way to get the actually changed items or at least the ids from MongoDB multi updates (or many other db adapters).
There was a problem hiding this comment.
ok cool. Thanks! I added some more detail to the comment for future reference.
There was a problem hiding this comment.
Maybe the better fix is to make a find, run the update and then another find with the list of original ids via find({ _id: { $in: oldFindIds } }) after.
There was a problem hiding this comment.
@daffl probably. That would be easier to trace and also the way I would normally do it in my own code. However, it might be more expensive an operation than just doing what you are doing in memory. Especially if patching a lot of objects. We'd almost need to test with 10k documents or something.
A common use case for patches like that are database migrations so large numbers would be common.
There was a problem hiding this comment.
We could add a flag for that case. But in order to get real-time events there isn't really a way around returning all changed items. I think it makes sense to change the adapters to what I suggested because then it'll at least return only the changed items (right now it returns everything that matches the query).
There was a problem hiding this comment.
Oh I didn't notice that. You're right. It is better to do that. At least then we are dispatching for the correct records. We can optimize for speed later (it might not be bad, it could just use some testing).
|
@daffl Thanks for fixing this! With this many changes I would have liked to review before shipping even though tests are green. Ping me next time please. 😄 |
|
True, sorry. It's not shipped yet so we can still fix it. |
Hello lovely humans,
feathers-service-tests just published its new version 0.8.1.
This version is not covered by your current version range.
Without accepting this pull request your project will work just like it did before. There might be a bunch of new features, fixes and perf improvements that the maintainers worked on for you though.
I recommend you look into these changes and try to get onto the latest version of feathers-service-tests.
Given that you have a decent test suite, a passing build is a strong indicator that you can take advantage of these changes by merging the proposed change into your project. Otherwise this branch is a great starting point for you to work on the update.
Do you have any ideas how I could improve these pull requests? Did I report anything you think isn’t right?
Are you unsure about how things are supposed to work?
There is a collection of frequently asked questions and while I’m just a bot, there is a group of people who are happy to teach me new things. Let them know.
Good luck with your project ✨
You rock!
🌴
The new version differs by 13 commits .
99b35db0.8.10e63d0bAdd test for sorting with strings (#22)3f6cc200.8.0925e379Merge pull request #20 from feathersjs/multi-patchaf1add3Add tests for proper multi patch9c14ffa0.7.11588d8aMerge pull request #19 from feathersjs/multi-id-prop0f6e010Verify id property for multiple updates77d8ad20.7.04b62969Improved shared service tests (#18)29af2c9Upgrade to Mocha 3.x (#17)6e93948chore(package): update mocha to version 3.0.0 (#16)6da088achore(package): update request-promise to version 4.0.0 (#15)See the full diff.