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

Fixes #1582 - Gets mock login to work #1588

Merged
merged 1 commit into from
Jun 15, 2017

Conversation

brizental
Copy link
Contributor

So, I got mock login to work. I don't know if this is the best way, but it works. Let me know what you guys think.

The login is working, but the user activity page is not, it gets a "Bad credentials" error. I'll work on that tomorrow. I think it should be something close to what we already do with the issues fixtures.

Also, I noticed that even though now the login doesn't require remote server requests, there are some other tests that still do. Maybe it would be best to change this issue's name to "Get all tests to work offline" or something. What do you think?

Anyways, r? @miketaylr

@brizental
Copy link
Contributor Author

I just saw that along with this commit there is a package-lock.json file. I never saw that, but from a quick Google search I saw that it should be ignored... Should we add it to .gitignore? I can open a new issue for that.

Copy link
Member

@miketaylr miketaylr left a comment

Choose a reason for hiding this comment

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

This is clever. :)

@@ -0,0 +1,5268 @@
{
"name": "webcompat",

This comment was marked as abuse.

return github.authorize('public_repo')
if app.config['TESTING']:
session['username'] = 'testuser'
session['avatar_url'] = '/img/avatar.png?'

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@miketaylr
Copy link
Member

I just saw that along with this commit there is a package-lock.json file. I never saw that, but from a quick Google search I saw that it should be ignored... Should we add it to .gitignore? I can open a new issue for that.

oops, I should have read this question before commenting as much in review. Yeah, let's remove it and add a commit to .gitignore it.

@miketaylr
Copy link
Member

Maybe it would be best to change this issue's name to "Get all tests to work offline" or something. What do you think?

Let's open a new issue for that, and list which tests require network access.

The login is working, but the user activity page is not, it gets a "Bad credentials" error. I'll work on that tomorrow. I think it should be something close to what we already do with the issues fixtures.

Yeah, there should be a way to detect that we're in test mode, and just return some JSON fixture file that has the right content.

@brizental
Copy link
Contributor Author

r? @miketaylr

Let me know if you find anything else I need to change. I tested and everything seems to be working fine, but since I started using the notebook that Mozilla sent I have been getting the #1569 error, so I wasn't able to run all tests...

@miketaylr
Copy link
Member

@brizental can you rebase and push your branch back up before we merge this in?

git pull --rebase upstream master (assuming you have an upstream remote pointing to webcompat repo)
git push -f

@miketaylr
Copy link
Member

ok, looks good! i rebased everything down to a single commit. let's talk about rebasing commits in our next 1:1. it's possible to do some cool stuff. :)

(but sometimes it can get crazy....)

else:
# manually set the referer so we know where to come back to
# when we return from GitHub
set_referer(request)

This comment was marked as abuse.

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.

3 participants