Enhancing users model tests #684
Conversation
| user.remove(function(err) { | ||
| should.not.exist(err); | ||
| user.save(function(err) { | ||
| user3.email = 'test@test.com'; |
There was a problem hiding this comment.
Should this line be using the email property of user? If for some reason the value of user.email changes, it would be easier to manage by referencing whatever email is being used by the user object.
user3.email = user.email;
There was a problem hiding this comment.
yeah, good note, I'll update.
|
These tests are looking really good. I left a comment suggesting a minor change. |
|
@lirantal can we already make the indentation here 2 spaces with the expectation that we'll fix all other files soon? |
|
LGTM |
|
LGTM as well. Anyone have any thoughts on my line comment? |
|
@mleanos Personally, I write every test as if had to run on its own. I would do a beforeEach and afterEach so I don't rely on other tests to prove what the current test is doing. But for a middle ground, your solution would work nicely just in case another test further up ends up changing the email. |
|
@codydaig I agree with you on the strategy of using beforeEach and afterEach. It would ensure each test will be completely independent. I saw there was a discussion on moving this way, in some other issue. Can't seem to find it now. Thanks for providing your perspective. If we are moving to using this strategy, then my solution wouldn't be necessary; even as a temporary middle-ground. I'm definitely fine with this PR being merged, in it's current state. @lirantal @ilanbiala |
|
@codydaig Thanks. I actually just saw that you referenced it in this conversation. Too many tabs open, and it was right under my nose. |
|
@mleanos I go through the issues and pull requests when I'm on the train every morning and on the way home in attempt to better understand the structure and concepts (since It's hard to code on a tablet while standing, I read the issues and pull requests instead), so sadly I've got too many of these memorized. :P |
lirantal
force-pushed
the
feature/users_tests_email_unique
branch
from
602d5e5 to
ac35f0f
Compare
|
fixed, and squashed commits. |
Adding couple more tests to confirm users model works as expected.
Also confirms PR #439 is working as expected.