balderdashy / waterline-adapter-tests

API integration tests for Waterline adapters
MIT License
16 stars 41 forks source link

Adds tests for SELECT modifier #71

Closed dmarcelino closed 9 years ago

dmarcelino commented 9 years ago

As mentioned in https://github.com/balderdashy/waterline/issues/366#issuecomment-102396946 this PR adds tests for the SELECT modifier.

Unfortunately, as happened previously on PR #53 sails-mongo is blocking this from being merged as it breaks the tests: https://travis-ci.org/balderdashy/waterline-adapter-tests/jobs/62718504#L545.

Remaining task:

dmarcelino commented 9 years ago

As a side note, given balderdashy/waterline#997 we probably should add .find({ select: []}).populate() tests to the Associations interface to confirm associated records are not impacted by the select modifier.

dmarcelino commented 9 years ago

All tests passing, this one is ready merge!

@devinivy, mind giving this a look?

devinivy commented 9 years ago

Looks good! Brings up a question– if you findOne using select then save the record, are those undefined fields updated to be empty or ignored by waterline?

Also, does it make more sense to use assert.notProperty(x, y) or assert.isUndefined(x[y]) instead of assert.equal(x[y], undefined)? In any case, these tests look good to me.

dmarcelino commented 9 years ago

if you findOne using select then save the record, are those undefined fields updated to be empty or ignored by waterline?

I believe waterline ignores undefined fields but saves null fields.

Also, does it make more sense to use assert.notProperty(x, y) or assert.isUndefined(x[y]) instead of assert.equal(x[y], undefined)? In any case, these tests look good to me.

I'm not familiar with assert.notProperty() or assert.isUndefined(), nor I see them in Node.js docs... but you do have a point. Perhaps we should use assert('y' in x === false)?

dmarcelino commented 9 years ago

@devinivy, I've added the function assertNotProperty which asserts the property is not present in the retrieved record. It makes the tests more strict and also more coherent with the intended behaviour. Tests still pass for all official adapters which is reassuring :grinning:

devinivy commented 9 years ago

Awesome! I'd say merge away! I was getting confused with the Chai assertion library.

dmarcelino commented 9 years ago

Fair enough, merged, and thanks for the feedback! :wink: