Add python 3.3 to python and allow_failures #148 #149

Closed
wants to merge 14 commits into from

3 participants

@graingert

No description provided.

@graingert

Don't merge this until unittest2 problems are solved

@graingert graingert remove dependency on unittest2
This allows support for both python 2.6 and 3.3
de34807
@graingert

The skip_if function was taken from the MIT licensed https://github.com/eventlet/eventlet/blob/master/tests/__init__.py

In my opinion not a "significant portion of the code"

@willkg
Mozilla member

We're already using nose and we only need a skip_if that takes a boolean. Something like this is probably better:

from functools import wraps

from nose.plugins.skip import SkipTest


def skip_if(condition):
    """Skips tests if condition is True.
    """
    def skipped_wrapper(func):
        @wraps(func)
        def wrapped(*args, **kwargs):
            if condition:
                def skip_function(*args, **kwargs):
                    raise SkipTest()
                return skip_function
            return func(*args, **kwargs)
        return wrapped
    return skipped_wrapper

I haven't tested that, but the gist is there and it's in the style of the rest of django-browserid.

@willkg
Mozilla member

This is awesome work! I'm really psyched about supporting python 3.3 and I'm glad someone else is doing it because I just haven't had time, yet.

@graingert

It looks like with @willkg's change only 45 tests are run even under django 1.5 (not 47 as expected)

It's also broken with my skip_if fix, looks like it breaks classes

@willkg willkg commented on an outdated diff Apr 7, 2013
django_browserid/tests/__init__.py
@@ -87,3 +88,18 @@ def inner(*args, **kwargs):
with self:
return func(*args, **kwargs)
return inner
+
+
+def skip_if(condition):
+ """Skips tests if condition is True.
+ """
+ def skipped_wrapper(func):
+ @wraps(func)
+ def wrapped(*args, **kwargs):
+ if condition:
+ def skip_function(*args, **kwargs):
+ raise SkipTest()
+ return skip_function
@willkg
Mozilla member
willkg added a note Apr 7, 2013

This might need to be:

if condition:
    raise SkipTest()
return func(*args, **kwargs)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@graingert

Woo, now we support Python 3.3! (at least on paper)

@Osmose
Mozilla member

This looks good to me, if @willkg agrees I'll merge this in. :D

@graingert Oh yeah, can you add 3.3 back in as an allowed failure? I want to have at least one release with it as an allowed failure before I make it mandatory.

@willkg
Mozilla member

Super sorry about not seeing this until just now. I'll check it out tomorrow.

@willkg willkg and 1 other commented on an outdated diff May 10, 2013
install:
+ - pip install -e git+git://github.com/sobotklp/django-nose@b3f485e914965e0a7b7012c309864d6a6dcac3ed#egg=django-nose
@willkg
Mozilla member
willkg added a note May 10, 2013

What's this line for? django-nose is already in requirements.txt.

That's because django-nose by default does not support py3k, I'll add this to requirements.txt instead

@willkg
Mozilla member
willkg added a note May 10, 2013

Oh, hrm... It'd be better if we were using a released version of django-nose. Erik maintains django-nose. It's probably a good idea to push python 3 support in there. Having said that, I just remembered that "go through the django-nose python 3 support patches" is in my queue of things to do. :(

I don't think it should get added to requirements.txt since it's not something you need to have installed to use django-browserid. We should have a requirements-dev.txt file which has development-centric things in it.

@willkg
Mozilla member
willkg added a note May 10, 2013

Ahh... ok. This is different than all the other projects I work on--but that's fine. Awesome!

I'll get to the django-nose stuff next week. Whenever that settles out, I'll do a PR to update django-browserid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@willkg willkg commented on an outdated diff May 10, 2013
django_browserid/auth.py
@@ -29,7 +33,9 @@ def default_username_algo(email):
# this protects against data leakage because usernames are often
# treated as public identifiers (so we can't use the email address).
username = base64.urlsafe_b64encode(
- hashlib.sha1(email).digest()).rstrip('=')
+ hashlib.sha1(
+ smart_bytes(email)
+ ).digest()).rstrip(b'=')
@willkg
Mozilla member
willkg added a note May 10, 2013

Nit: I think these three lines could all be on the same line. It'd be easier to read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@willkg willkg commented on an outdated diff May 10, 2013
django_browserid/forms.py
@@ -19,7 +23,7 @@ class Media:
def clean_assertion(self):
try:
- return str(self.cleaned_data['assertion'])
+ return smart_bytes(self.cleaned_data['assertion'], encoding='ascii')
@willkg
Mozilla member
willkg added a note May 10, 2013

Nit: This line is too long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@willkg willkg commented on an outdated diff May 10, 2013
django_browserid/tests/test_auth.py
@@ -105,34 +99,34 @@ def test_verify_called_with_browserid_extra(self, user_verify):
user_verify.assert_called_with('asdf', 'asdf', extra_params=dic)
+if get_user_model:
# Only run custom user model tests if we're using a version of Django that
# supports it.
@willkg
Mozilla member
willkg added a note May 10, 2013

Nit: These two lines should either go before the if get_user_model: or be indented 4 spaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@willkg willkg commented on an outdated diff May 10, 2013
requirements.txt
@@ -5,7 +5,6 @@ fancy_tag==0.2.0
mock>=0.8.0
Django>=1.3
django-nose
-unittest2==0.5.1
@willkg
Mozilla member
willkg added a note May 10, 2013

Yay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@willkg willkg commented on an outdated diff May 10, 2013
@@ -5,7 +5,7 @@
def read(*parts):
- return codecs.open(os.path.join(os.path.dirname(__file__), *parts)).read()
+ return codecs.open(os.path.join(os.path.dirname(__file__), *parts), encoding="utf8").read()
@willkg
Mozilla member
willkg added a note May 10, 2013

Nit: We use single quotes instead of double-quotes to denote strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@willkg willkg commented on an outdated diff May 10, 2013
@@ -6,7 +6,16 @@ env:
- DJANGO_VERSION=1.3.7
- DJANGO_VERSION=1.4.5
- DJANGO_VERSION=1.5
+matrix:
+ include:
+ - python: 3.2
+ env: DJANGO_VERSION=1.5
+ - python: 3.3
+ env: DJANGO_VERSION=1.5
@willkg
Mozilla member
willkg added a note May 10, 2013

The 1.5 in this file should probably be 1.5.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@willkg willkg commented on an outdated diff May 10, 2013
install:
+ - pip install -e git+git://github.com/sobotklp/django-nose@b3f485e914965e0a7b7012c309864d6a6dcac3ed#egg=django-nose
- pip install -e git+git://github.com/django/django.git@${DJANGO_VERSION}#egg=django
@willkg
Mozilla member
willkg added a note May 10, 2013

Instead of pulling from git, I think you can do:

pip install django=={DJANGO_VERSION}

Would that work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@willkg
Mozilla member

I can't seem to get a functional python3.3 virtual environment set up for some reason.

I mentioned a few nits some of which should get fixed while others are subjective. Other than that, it looks ok to me.

@Osmose
Mozilla member

Merged in ee8a1e3. Nice job, @graingert ! 👍

@Osmose Osmose closed this May 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment