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

#141393367 Fixed accounts tests #3

Merged
merged 17 commits into from
Mar 28, 2017
Merged

Conversation

bmulobi
Copy link
Contributor

@bmulobi bmulobi commented Mar 24, 2017

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 80.042% when pulling a6b0975 on ft-add-accounts-tests-141393367 into 8279464 on master.

Copy link
Contributor

@abiodun0 abiodun0 left a comment

Choose a reason for hiding this comment

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

Excellent work from you guys. Just some minor nitpicks about naming conventions and not repeating what doesn't need to be repeated

@@ -15,14 +15,29 @@ def test_it_shows_form(self):

def test_it_redirects(self):
r = self.client.post("/accounts/check_token/alice/secret-token/")
self.assertEqual(r.status_code, 302)
# self.assertIn(self.profile.user, self.client.session)
Copy link
Contributor

Choose a reason for hiding this comment

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

CodeQuality: Nitpick
Remove commented out code

self.profile.refresh_from_db()
self.assertEqual(self.profile.token, "")

### Login and test it redirects already logged in
def test_redirects_already_logged_in(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Code Quality: Nitpick
this should be written like so
def test_redirects_if_already_logged_in

@@ -21,12 +22,21 @@ def test_it_sends_link(self):

### Assert that a user was created

user_check = User.objects.filter(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.

CodeQuality: Nitpick
Since you are getting only one value from the database this would be better as this
user = User.objects.get(email="alice@example.org")
So you avoid repitation on line 36

@@ -42,9 +52,16 @@ def test_it_adds_team_member(self):

### Assert the existence of the member emails

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

Choose a reason for hiding this comment

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

CodeQuaility: Nitpick
This code should be removed as it's just a smoke test

@coveralls
Copy link

coveralls commented Mar 28, 2017

Coverage Status

Coverage increased (+0.4%) to 80.042% when pulling 3ca568f on ft-add-accounts-tests-141393367 into 8279464 on master.

@abiodun0 abiodun0 merged commit 9f2ea0b into master Mar 28, 2017
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

3 participants