Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds tests for SELECT modifier #71

Merged
merged 2 commits into from
Jun 7, 2015

Conversation

dmarcelino
Copy link
Member

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:

  • Fix sails-mongo balderdashy/sails-mongo#281

@dmarcelino
Copy link
Member Author

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
Copy link
Member Author

All tests passing, this one is ready merge!

@devinivy, mind giving this a look?

@devinivy
Copy link
Contributor

devinivy commented Jun 7, 2015

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
Copy link
Member Author

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
Copy link
Member Author

@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 😀

@devinivy
Copy link
Contributor

devinivy commented Jun 7, 2015

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

dmarcelino added a commit that referenced this pull request Jun 7, 2015
Adds tests for SELECT modifier
@dmarcelino dmarcelino merged commit 2bce59c into balderdashy:master Jun 7, 2015
@dmarcelino dmarcelino deleted the select-tests branch June 7, 2015 18:17
@dmarcelino
Copy link
Member Author

Fair enough, merged, and thanks for the feedback! 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants