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

Convert timeout argument to int values when applicable #2050

Closed
wants to merge 7 commits into from

Conversation

RaHus
Copy link
Contributor

@RaHus RaHus commented Oct 28, 2015

In response to #1656.

Reapplied the changes on top of master.

@stevepiercy
Copy link
Member

@RaHus as an aside, I recently added contributing.md, which appears when creating a new issue on GitHub. It's intended for new contributors to Pyramid.

  1. Did you notice it?
  2. If so, do you have suggestions for improvement?
  3. If not, where did you find how to do a proper PR? Yours is the first one in a long time that includes a signed CONTRIBUTORS.txt.

Thanks.

@RaHus
Copy link
Contributor Author

RaHus commented Oct 28, 2015

@stevepiercy yeah i did notice it and it did include helpful information, thanks :)
A possible improvement could be a list with the long lived branches of the project and a small description on how they fit in the pyramid git workflow.

@mmerickel
Copy link
Member

Can I convince you to fix timeout on the AuthTktAuthenticationPolicy[1] as well?

Have you investigated whether this is also an issue with the other parameters (reissue_time and max_age)?

[1] https://github.com/Pylons/pyramid/blob/master/pyramid/authentication.py#L858

@RaHus
Copy link
Contributor Author

RaHus commented Oct 28, 2015

@mmerickel Sure you can, i am glad i can help :).

It looks like reissue_time is affected the same as timeout but i didn't have time to test it properly.

Next chance i get i'll investigate further and include the changes in this PR if needed.

@digitalresistor
Copy link
Member

max_age is int()ed in WebOb. Doing it sooner would probably help people figure out what is going on sooner rather than later.

@mmerickel
Copy link
Member

@RaHus I'd appreciate it very much, thank you. As @bertjwregeer said, max_age would be good to check as well as it isn't checked until a request occurs as well.

@RaHus
Copy link
Contributor Author

RaHus commented Oct 29, 2015

@mmerickel UnencryptedCookieSessionFactoryConfig doesn't support the reissue_time parameter, so i followed a previous authors example and copy/pasted the tests for the subclasses that support reissue_time.
To keep things D.R.Y, since UnencryptedCookieSessionFactoryConfig is being deprecated and is alone in not supporting reissue_time maybe its better to keep all the reissue_time tests in SharedCookieSessionTests and override the functions of the problematic tests in TestUnencryptedCookieSession. That means future authors of reissue tests would need to maintain this list up to date, so i am not decided if that is indeed a better solution.
Another possibility to avoid overriding in TestUnencryptedCookieSession would be to wrap the _makeOne calls in the relevant tests with a try block, catch and ignore the TypeError exception if its coming from UnencryptedCookieSessionFactoryConfig.
The same holds true for max_age tests, although no other max_age tests exist from what i can see.

@RaHus
Copy link
Contributor Author

RaHus commented Nov 3, 2015

TravisCI fails due to missing ordereddict in python2.6 which is required by WebTest. see #2071

@tseaver
Copy link
Member

tseaver commented Nov 3, 2015

The failure is due to a WebTest packaging bug.

@tseaver
Copy link
Member

tseaver commented Nov 3, 2015

The wheel is gone: I restated the failed 2.6 test, and it passed.

@RaHus
Copy link
Contributor Author

RaHus commented Nov 3, 2015

I think i have included all the items that where discussed here. Any comments on the PR so far or any other item/changes/corrections i should consider? I would love to finalize this and pick another issue to work on.

@@ -758,6 +767,12 @@ def test_identify_cookie_timed_out(self):
result = helper.identify(request)
self.assertEqual(result, None)

def test_identify_cookie_str_timeout(self):
Copy link
Member

Choose a reason for hiding this comment

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

How does this test the timeout of the cookie? A bogus auth_tkt is already invalid and will always "timeout".

@RaHus
Copy link
Contributor Author

RaHus commented Nov 5, 2015

The rationale was to make sure it would behave the same with a valid str and int mostly due to me being unfamiliar with the code, but i see how this is needless. Prior to that i was just doing:

def test_init_cookie_str_timeout(self):
    helper = self._makeOne('secret', timeout='1')
    self.assertTrue(helper.timeout == 1)

I followed the same rationale for other valid string parameter tests which i will change once someone confirms that the above (just check that the conversion to int is done) is enough

@digitalresistor
Copy link
Member

Yes, what you put down is enough. If you make that change I'll be happy to +1 it.

digitalresistor added a commit that referenced this pull request Nov 13, 2015
@digitalresistor
Copy link
Member

Thank you @RaHus, I've pulled the changes and made the modifications. Please see #2117

mmerickel added a commit that referenced this pull request Nov 13, 2015
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