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

Added tests for Login #41

Merged
merged 6 commits into from
Mar 19, 2018
Merged

Conversation

rosygupta
Copy link
Collaborator

#29
Added tests for login. Some doubts and issues:

  1. Currently, vcrpy installation using pipenv gives me error as discussed with @gedankenstuecke . So I intsalled vcrpy locally using pip and coded. Would be lovely if somebody else could add the vcrpy to the Pipfile for the same to work.
  2. Also, I'm not sure if vcr is required. Since the login view test (test_login_success) can perform the same functionality.

request_url = self.project_info_url.format(self.invalid_token)
response = requests.get(request_url)
self.assertEqual(response.status_code, 401)

Choose a reason for hiding this comment

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

blank line contains whitespace

@rosygupta
Copy link
Collaborator Author

rosygupta commented Mar 18, 2018

Codeclimate is raising an issue regarding refactoring which I feel isn't possible to fix. Travis issue will be solved when vcrpy will be added by somebody as I mentioned in the PR description.

@mcescalante
Copy link
Member

@rosygupta can you add vcrpy to the Pipfile in this PR? If you add it, the Travis build should use the Pipfile from here. The reason it's not currently there is because you're the first to submit vcrpy tests for this repo 😄

@rosygupta
Copy link
Collaborator Author

rosygupta commented Mar 19, 2018

@mcescalante can you please refer to my discussion with @gedankenstuecke on slack? As I already mentioned in the PR description here, I am facing some issue with pipenv install on my local system. Seems like a known issue with pipenv and vcrpy -- raised recently on their github handle. pypa/pipenv#1617
Therefore, I couldn't waste more time on fixing it and coded it and requesting someone else to add vcrpy to this. :D

@rosygupta
Copy link
Collaborator Author

rosygupta commented Mar 19, 2018

@mcescalante The issue I was facing, its the same issue that is hitting Travis and its failing the build. This seems like a legit bug with pipenv and vcrpy combination.
For reference: kevin1024/vcrpy#342 (comment)
pypa/pipenv#1617 (comment)

@mcescalante
Copy link
Member

@rosygupta yeah I can see that, I just read through the issues. I'll probably leave this until a bit later this afternoon when I have a minute to see if I can dig further into it. Odd issue!

@gedankenstuecke
Copy link
Member

Ok, I added the Pipfile.lock that works for me and I also fixed the collectstatic bug by just adding settings.DEBUG = True to the Up command for the tests. That way the static files are automatically delivered like in ./manage.py runserver. Hope that helps.

@mcescalante mcescalante merged commit 50d93cb into OpenHumans:master Mar 19, 2018
def test_invalid_token(self):
request_url = self.project_info_url.format(self.invalid_token)
response = requests.get(request_url)
self.assertEqual(response.status_code, 401)
Copy link
Contributor

Choose a reason for hiding this comment

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

@rosygupta I have a doubt on why would we require these tests, Usually tests are written to test the lines that we wrote or we could change. But here the test_invalid_token and test_valid_token are testing the API interface of openhumans which doesn't belong to this project (It belongs to https://github.com/OpenHumans/open-humans ), it's behaviour changes only when something in the OpenHumans main repository is changed.
So what could be a reason behind dropping these two tests in here in oh-proj-management? when these tests doesn't test anything related to oh-proj-management code?
Let me know if I am wrong in my understanding. Thank You.

Copy link
Member

@mcescalante mcescalante Mar 20, 2018

Choose a reason for hiding this comment

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

@eeshadutta In order to use this app, oh-proj-management, it is required to enter a (valid) token in. Because our app oh-proj-management does check token validity in order to decide on login success or failure, it makes sense to test that our communications between oh-proj-management and the main site (open-humans) work as expected for both token exchanges and the actual login in our app (which also uses those tokens). Let me know if this clears things up at all, I would be happy elaborate!

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.

5 participants