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

feat(users): Edit Profile client controller tests #1329

Merged

Conversation

mleanos
Copy link
Member

@mleanos mleanos commented May 2, 2016

Adds client-side tests for the Users Edit Profile client controller.

  1. should have user context
  2. should update the user profile
  3. should set vm.error if error

Related #1283

Adds client-side tests for the Users Edit Profile client controller.

1) should have user context
2) should update the user profile
3) should set vm.error if error

Related meanjs#1283
@mleanos mleanos added this to the 0.5.0 milestone May 2, 2016
@mleanos mleanos self-assigned this May 2, 2016
@coveralls
Copy link

coveralls commented May 3, 2016

Coverage Status

Coverage remained the same at 70.615% when pulling 6b263b7 on mleanos:feature/users-client-update-profile-tests into 5da5a61 on meanjs:master.

$scope.vm.updateUserProfile(true);
$httpBackend.flush();

expect($scope.vm.success).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it sufficient to check if success is true? I think maybe it should also check if the properties of the mock users are really changed? Just trying to offer some feedback and learn a thing or 2 at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

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

If our controller sets the success view model property to `true', then we can assume our controller is behaving as expected. I don't see a need to test the actual back-end functionality here as well. We already have CRUD server tests for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I understand

@ilanbiala
Copy link
Member

@mleanos any progress with this?

@mleanos
Copy link
Member Author

mleanos commented May 15, 2016

@ilanbiala I think this is ready. I've just been waiting for others to give their approval. Is there something specific that seems to be outstanding with this?

@lirantal
Copy link
Member

@mleanos go ahead and merge then?

@mleanos mleanos merged commit b795ddc into meanjs:master Jun 25, 2016
@mleanos
Copy link
Member Author

mleanos commented Jun 25, 2016

Merged. Thanks @lirantal

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.

None yet

5 participants