Closed estilles closed 9 years ago
Yes, it's a good idea
Awesome! I will start working on this tomorrow. Will use Mongoose 4.x
for the tests.
I also suggest we implement Istanbul so we can keep track of the coverage.
@SamyPesse, while unit testing FilterablePlugin
I noticed the following:
.search()
returns an empty mongoose query, which makes it difficult to make timely assertionsQuery#parse
(via Filterable#query
)Query#parse
is Query#filterFields
Query#filterFields
is invoking user provided async middleware to resolve values
.I also noticed that:
.search()
does not accept a callback argument (as does mongoose' Model#find
and others)..search()
(as you currently can with Model#find
). Well technically you can, but the last .search()
winsThese last few would be nice to have, but they're not a deal breaker for me.
Did I understand all of this correctly or did I miss something?
If my observations are correct, then the only reason for Query#parse
to be async is the middleware to resolve values
. Everything else appears to by synchronous in nature. If we could figure out a way to processes these middleware synchronously, we could easily make .search()
behave more like its mongoose counterparts.
Just a thought.
In the meantime, I'll try to figure out a way to test FilterablePlugin
without making any dramatic changes to the existing code base.
I may have found a solution to the situation I described in my comment above. It will significantly simplify .search()
and will make it more testable. I will include it in my PR so we can review and discuss.
Unit tests for FiltarablePlugin
requires a complete refactoring of lib/mongoose.js
in my opinion. We should revisit this issue at a later time, when the code is refactored for testability.
I think before I continue work on PR #12 we should consider adding unit tests for
FilterablePlugin
. If you agree, I will submit a separate PR for this.