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

Bug fix accounts tests #1

Merged
merged 17 commits into from
Jan 5, 2017
Merged

Bug fix accounts tests #1

merged 17 commits into from
Jan 5, 2017

Conversation

ianoti
Copy link
Contributor

@ianoti ianoti commented Dec 14, 2016

The unit tests are in the five files under hc/accounts/tests.
The assigned tasks are under the comments in the hc/accounts/tests files.

@coveralls
Copy link

coveralls commented Dec 14, 2016

Coverage Status

Coverage increased (+0.6%) to 80.873% when pulling eb4e75d on bug-fix_accounts_tests into 5a9fc32 on develop.

@@ -22,7 +22,15 @@ def test_it_redirects(self):
self.assertEqual(self.profile.token, "")

### Login and test it redirects already logged in
def test_it_redirects_if_already_logged_in(self):
self.client.login(username="alice@example.org", password="password")
r = self.client.get("/accounts/check_token/alice/secret-token/")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use something more specific than r

@@ -20,18 +20,23 @@ def test_it_sends_link(self):
assert r.status_code == 302

### Assert that a user was created
self.assertEqual(len(User.objects.filter(email="alice@example.org")), 1)
Copy link
Contributor Author

@ianoti ianoti Dec 14, 2016

Choose a reason for hiding this comment

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

Better assertion would be to assert user data rather than length


### Assert that check is associated with the new user
self.test_user = User.objects.get(email="alice@example.org")
self.test_check = Check.objects.get(user=self.test_user.id)
self.assertEqual(self.test_user.id, self.test_check.user_id)

def test_it_pops_bad_link_from_session(self):
self.client.session["bad_link"] = True
self.client.get("/accounts/login/")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Try putting in a bad link that gets popped out

@@ -41,10 +51,15 @@ def test_it_adds_team_member(self):
member_emails.add(member.user.email)

### Assert the existence of the member emails
self.assertEqual(len(member_emails), 2) #Alice included as member

self.assertTrue("frank@example.org" in member_emails)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

DOn't hard code the email address

@@ -108,3 +123,20 @@ def test_it_shows_badges(self):
self.assertNotContains(r, "bobs-tag.svg")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assert that after adding a team member, an access allowed flag is present

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After reviewing the code, the user Charlie doesn't have team access enabled hence why the expected error code is a 403 because access is being denied. the test for successful adding of a team member is handled in test_it_adds_team_member and the expected status code is 200 since Alice has team access enabled.

@coveralls
Copy link

coveralls commented Dec 14, 2016

Coverage Status

Coverage increased (+0.6%) to 80.873% when pulling 595668c on bug-fix_accounts_tests into 5a9fc32 on develop.

@coveralls
Copy link

coveralls commented Dec 14, 2016

Coverage Status

Coverage increased (+0.6%) to 80.873% when pulling 312bf7b on bug-fix_accounts_tests into 5a9fc32 on develop.

@coveralls
Copy link

coveralls commented Dec 14, 2016

Coverage Status

Coverage increased (+0.6%) to 80.873% when pulling 4e67c3f on bug-fix_accounts_tests into 5a9fc32 on develop.

@coveralls
Copy link

coveralls commented Dec 14, 2016

Coverage Status

Coverage increased (+0.6%) to 80.873% when pulling 644d3a3 on bug-fix_accounts_tests into 5a9fc32 on develop.

@coveralls
Copy link

coveralls commented Dec 14, 2016

Coverage Status

Coverage increased (+0.6%) to 80.873% when pulling 644d3a3 on bug-fix_accounts_tests into 5a9fc32 on develop.

@coveralls
Copy link

coveralls commented Dec 14, 2016

Coverage Status

Coverage increased (+0.6%) to 80.873% when pulling 644d3a3 on bug-fix_accounts_tests into 5a9fc32 on develop.

Copy link
Contributor Author

@ianoti ianoti left a comment

Choose a reason for hiding this comment

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

I have worked on the changes. I believe the branch is ready for merging.

def test_it_redirects_if_already_logged_in(self):
self.client.login(username="alice@example.org", password="password")
response = self.client.get("/accounts/check_token/alice/secret-token/")
assert response.status_code == 302
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe some consistency with the tests. If you are asserting the status code maybe assert for all

Copy link
Contributor

Choose a reason for hiding this comment

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

this assert is the main python assert and not the one for the base test class

@@ -20,18 +20,23 @@ def test_it_sends_link(self):
assert r.status_code == 302

### Assert that a user was created
self.test_user = User.objects.get()
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a safe query when we have more than 1 user

@@ -20,18 +20,23 @@ def test_it_sends_link(self):
assert r.status_code == 302

### Assert that a user was created
self.test_user = User.objects.get()
self.assertEqual(self.test_user.email, "alice@example.org")
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a better logic to test is assert that there is no user with a particular email, then create a user with that email, then assert that a user with that email now exixts and that the count of User objects has incremented by one.

self.assertIsNotNone(self.alice.profile.api_key)
self.assertContains(response, "The API key has been created!")


Copy link
Contributor

Choose a reason for hiding this comment

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

what plans do you have for those two spaces?

### Assert the contents of r
### Assert the contents of response
self.assertEqual(response.status_code, 200)
self.assertTemplateUsed(response, "front/my_checks.html")


def test_it_checks_team_membership(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a better name for the test to have it less ambiguous

added uniform assertions for the redirect status code and PEP8 formatting

added uniform assertions for the redirect status code and PEP8 formatting

modified logic of testing for user creation and code for pep8 compliance

minor edits to code to make assertions uniform and pep8 compliance

minor edit to code for pep8 compliance and reworked logic for obtaining count of profile objects

removed hardcoding of email in assertion test
@mirabel-koso mirabel-koso merged commit 42eed03 into develop Jan 5, 2017
@mirabel-koso mirabel-koso deleted the bug-fix_accounts_tests branch January 5, 2017 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants