Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Tests - Adding route tests for user/admin CRUD operations #928

Merged
merged 1 commit into from
Sep 22, 2015

Conversation

lirantal
Copy link
Member

No description provided.

@lirantal lirantal self-assigned this Sep 21, 2015
@lirantal lirantal added this to the 0.4.2 milestone Sep 21, 2015
});
};

user = new User(_user);

// Save a user to the test db and create new article
user.save(function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we check that the user was saved and an error wasn't thrown?

@lirantal
Copy link
Member Author

@ilanbiala thanks, great feedback!
I addressed all the comments.

@lirantal
Copy link
Member Author

@ilanbiala any ideas why I don't see the coverage status here? did something break there?
We do see it for other PRs for example:
image

@ilanbiala
Copy link
Member

@lirantal I noticed that, I'm not sure exactly why but Coveralls had the last build as 1352. I just restarted the build so hopefully Coveralls will get that one.

@lirantal
Copy link
Member Author

Yep, now it's there, thanks.

@lirantal lirantal force-pushed the feature/user-route-tests-improve branch from 79715d2 to 6d9f01e Compare September 21, 2015 17:02
@lirantal
Copy link
Member Author

@ilanbiala updated and addressed all your comments.

@ilanbiala
Copy link
Member

@lirantal one more return missed that I didn't see before. https://github.com/meanjs/mean/pull/928/files#diff-f2da820fbb902b375a9e67035ac86c1bR152

Other than that, LGTM.

@lirantal
Copy link
Member Author

missed it, thanks!

@lirantal lirantal force-pushed the feature/user-route-tests-improve branch from 6d9f01e to de354ee Compare September 21, 2015 17:18
@ilanbiala
Copy link
Member

LGTM.

@codydaig
Copy link
Member

LGTM

lirantal added a commit that referenced this pull request Sep 22, 2015
Tests - Adding route tests for user/admin CRUD operations
@lirantal lirantal merged commit 658ac8d into meanjs:master Sep 22, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants