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

Figure out how to make logging in work for func tests #277

Closed
miketaylr opened this issue Sep 18, 2014 · 18 comments
Closed

Figure out how to make logging in work for func tests #277

miketaylr opened this issue Sep 18, 2014 · 18 comments

Comments

@miketaylr
Copy link
Member

Right now there's a problem preventing us from testing any of the non-homepage logged-in functionality, i.e., #270, #271, #272.

For whatever reason, Selenium/Intern isn't able to read the session cookie, where we store the next_url key which tells the browser where to go after going through the auth routine.

See https://github.com/webcompat/webcompat.com/blob/master/webcompat/views.py#L74
and https://github.com/webcompat/webcompat.com/blob/master/webcompat/views.py#L95.

So no matter what, you'll end up back at the homepage. Manually redirecting in the test won't help--it can't read the session to know that you're logged in.

@vladikoff suggested we try to return the cookie value to tests like in https://github.com/mozilla/fxa-content-server/blob/master/tests/functional/lib/helpers.js#L20. Worth exploring.

The other option would be to ditch the next_url param and storing next_url in the session entirely and pick another approach. We could store the URI of where the login request came from (in flask.g maybe?)

@karlcow
Copy link
Member

karlcow commented Sep 18, 2014

@miketaylr How the test suite is supposed to work?

I have seen

Many tests require the ability to log in with GitHub OAuth. This is achieved by passing in a GitHub username: user and password: pw as command-line arguments:

But does that mean the tests are done on the real instance of webcompat.com or on the local test instance. I don't see anything related to SetUp and TearDown for the functional tests where we could for example start the instance of the local server, create necessary issues for the test, and login information.

@karlcow
Copy link
Member

karlcow commented Sep 18, 2014

Hmmm searching…

Found Configuring Intern to setup/teardown my server mock

@miketaylr
Copy link
Member Author

Here's how I run tests:

testw () {
  echo "Hold on to your butts."
  wd="/Users/mtaylor/dev/compat/webcompat.com"
  osascript -e "tell application \"Terminal\" to do script \"cd $wd; . env/bin/activate && python run.py\"" > /dev/null
  osascript -e "tell application \"Terminal\"" \
            -e "tell application \"System Events\" to keystroke \"t\" using {command down}" \
            -e "do script \"cd $wd;java -jar selenium-server-standalone-2.42.2.jar\" in front window" \
            -e "end tell" > /dev/null
  osascript -e "tell application \"Terminal\" to do script \"cd $wd; sleep 1; node_modules/.bin/intern-runner config=tests/functional/intern user=$TESTUSER pw=$TESTPW\"" > /dev/null
}

I've exported $TESTUSER and $TESTPW in my .bash_profile.

Right now we don't have any kind of setUp/tearDown stuff--but that would be a nice place to put the login routine. There's some of that going on in https://github.com/mozilla/fxa-content-server/blob/master/tests/functional/complete_sign_up.js, for example.

Here's where user and pw are defined:
https://github.com/webcompat/webcompat.com/blob/master/tests/functional/intern.js#L16-L19

@miketaylr
Copy link
Member Author

As part of this, I was working on removing ?next_url for login/logout links and using Referer instead. I can push it to a branch--I stopped when I realized that GitHub flagged us as spammers. Feedback would be good.

@miketaylr
Copy link
Member Author

Here's that commit: a1e7892 on the referer branch.

Everything works when I test it manually, but I'm hesitant to merge it in because it 4 tests are failing. Unsure if that's due to these changes (which could be OK, if the tests were written to the old behavior) or to the recent banning of all our bots cough cough

@miketaylr
Copy link
Member Author

a1e7892 doesn't solve all the problems though--we're still using session[user_id] as a signal that the user is logged in in the authorized handler (and set g.user based on that)....

@miketaylr
Copy link
Member Author

I wondered if Selenium was struggling with reading/setting our session cookies. I still don't know, but something is weird. Make sure you pip install selenium and have the local webcompat server running:

from selenium import webdriver

driver = webdriver.Firefox()
driver.get("http://localhost:5000/issues/100")
elem = driver.find_element_by_css_selector(".nav__section--right .nav__link")
elem.click()

This will navigate to Issue 100 and click the login link. You should now be at GitHub, so you can type in your user credentials.

Expected: redirected back to Issue 100
Actual: redirected to homepage.

If you try this from a regular web browser, new session, private window, whatever--it redirects back to the right place. 💣

@miketaylr
Copy link
Member Author

OK. I think the root of the issue is SESSION_COOKIE_HTTPONLY defaulting to True in Flask (a good thing). So Selenium can't interact with these because at some point it's just JS manipulating a web page (c.f., http://www.aosabook.org/en/selenium.html). And httponly cookies are not visible to JS.

I wonder if we can disable this for the session cookie only for testing. It might be a good approach. It could mean ditching Intern to more easily manipulate this from python. Or we could could pass an argument to run.py to let it know it's in testing mode. Possibly something to do in the meantime, as a quick fix.

@karlcow
Copy link
Member

karlcow commented Sep 19, 2014

Some explorations

[13:57]  <karlcow> https://github.com/horejsek/python-webdriverwrapper/ last commit July 28, 2014
[14:04]  <karlcow> http://pythonhosted.org/behave/
[14:06]  <karlcow> http://splinter.cobrateam.info/
[14:07]  <karlcow> splinter looks nice at first sight

@miketaylr
Copy link
Member Author

Related to this work, #279

It doesn't solve everything though.

@karlcow
Copy link
Member

karlcow commented Nov 6, 2014

@miketaylr I was wondering why do we need to login or even make request to github. What we want is to test the functionality. Is it possible/hard to mock up and then not rely on network requests for testing?

@miketaylr
Copy link
Member Author

Logging in is something we want to make sure we don't break. We also want to make sure that certain features stay behind a login. Or that not-logged in people can do the same thing with our proxy request stuff, etc. So I don't think there's any way to test the appliction well without dealing with logins.

Is it possible/hard to mock up and then not rely on network requests for testing?

It should be possible to mock logging in, but probably a little complicated to mock OAuth2 token exchange.

http://stackoverflow.com/questions/18827985/mocking-oauth-providers-while-testing looks interesting.

@karlcow
Copy link
Member

karlcow commented Nov 7, 2014

Adding also Flask Testing Client.

Let me clarify what I meant. I agree that testing logging is important, but it doesn't mean we have to actually log on github.

Logging is done through an exchange, a negotiation in between two entities. What we want to be sure is that our application reacts well to the supposed message it receives (be a redirection URL, be a cookie, etc, choose your message). By mocking up the response, I meant define the supposed returned response so we are not dependent of online interactions (isolated tests environment), so our side of the application succeeds and fails well.

OAuth2 seems indeed a bit more complex. I searched a couple of days ago about OAuth testing + intern and failed to find anything really interesting.

All of that with a tongue-in-cheek, because I have no idea (yet) how to setup this.

miketaylr pushed a commit that referenced this issue Nov 18, 2014
miketaylr pushed a commit that referenced this issue Nov 18, 2014
Issue #277 - Upgrade Intern to 2.1.1
@miketaylr
Copy link
Member Author

I've spent a few hours messing with this again and it's time to give up for now.

Observations:

In test/functional/libs/index.js, when the first test logs in it succeeds. The second time fails.

This my local server. From /issues/22 and when not logged in, I click "Login". session['user_id'] gets set in the /callback route, and I'm logging right after that (why it's non-None).

Inside of the /issues/<int:number> route, I log g.user and session[user_id]. Normal, everything works as expected.

127.0.0.1 - - [18/Nov/2014 14:09:01] "GET /login HTTP/1.1" 302 -
('g.user', None)
("session.get('user_id')", 1)
('redirect to referer now', 'http://localhost:5000/issues/22')
127.0.0.1 - - [18/Nov/2014 14:09:04] "GET /callback?code=f769dac41aeb0724f2e3 HTTP/1.1" 302 -
('g.user', <webcompat.models.User object at 0x10c5f1750>)
("session.get('user_id')", 1)
127.0.0.1 - - [18/Nov/2014 14:09:04] "GET /issues/22 HTTP/1.1" 200 -

Now, when I run the same thing from Intern:

127.0.0.1 - - [18/Nov/2014 14:15:35] "GET /login HTTP/1.1" 302 -
('g.user', None)
("session.get('user_id')", 1)
('redirect to referer now', 'http://127.0.0.1:5000/issues/100')
127.0.0.1 - - [18/Nov/2014 14:15:35] "GET /callback?code=77ebba067f4ba94d97a6 HTTP/1.1" 302 -
('g.user', None)
("session.get('user_id')", None)
127.0.0.1 - - [18/Nov/2014 14:15:35] "GET /issues/100 HTTP/1.1" 200 -

The user_id was correctly set in the session from the /callback handler.

But for whatever reason, the session (cookie) is lost between GET requests (g.user is set from the user_id key of the session object in @app.before_request).

I need to do more digging on the Selenium/Intern side of things--rewriting the way we handle sessions on the server side doesn't (appear) to make a difference.

@miketaylr
Copy link
Member Author

💟

[17:10] <vladikoff> yoooo
[17:11] <vladikoff> are you sure your login bug is not 127.0.0.1:5000 vs localhost:5000 cookie issue
[17:14] <miketaylr> NOPE
[17:14] miketaylr adds to things to test
[17:14] <vladikoff> i noticed in the webcompat test 
[17:14] <miketaylr> i verified that github-flask has nothing to do with it
[17:14] <vladikoff> starting at 127.0.0.1 then redirecting to localhost
[17:14] <miketaylr> hm
[17:21] <miketaylr> asdlkfja;sldkfn;oasi sdjfk

😍

Silly mistake which stole most of my day. Thanks @vladikoff.

@karlcow
Copy link
Member

karlcow commented Nov 18, 2014

Supercalifragilisticexpialidocious!

miketaylr pushed a commit that referenced this issue Nov 19, 2014
@miketaylr
Copy link
Member Author

And here's the why: https://github.com/webcompat/webcompat.com/blob/master/config.py.example#L66

If we start at 127.0.0.1 and login, Github will redirect us back to localhost. Because that's what we've told it to do. So the origin changed and presumably we don't have access to the session cookie at that point. 🙈 🙉 🙊

@miketaylr
Copy link
Member Author

This is fixed, forgot to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants