Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py

 local variable 'user' is assigned to but never used

Col: 41
 E126 continuation line over-indented for hanging indent

 local variable 'get_credentials_mock' is assigned to but never used

 local variable 'get_credentials_mock' is assigned to but never used

Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py



Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py

Col: 53
 E126 continuation line over-indented for hanging indent

Col: 9
 E265 block comment should start with '# '

Col: 41
 E126 continuation line over-indented for hanging indent

Col: 41
 E126 continuation line over-indented for hanging indent

Col: 41
 E126 continuation line over-indented for hanging indent

Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py



Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py

Col: 53
 E126 continuation line over-indented for hanging indent

Col: 41
 E126 continuation line over-indented for hanging indent

Col: 41
 E126 continuation line over-indented for hanging indent

Col: 41
 E126 continuation line over-indented for hanging indent

Col: 41
 E126 continuation line over-indented for hanging indent

Col: 13
 E128 continuation line under-indented for visual indent

Col: 13
 E128 continuation line under-indented for visual indent

Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py



Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py

You don't need the entire module path in here, you can just do:

"""Unit tests for the WebAPITokenBackend."""


For future reference, docstrings should be of the form:


"""Single line sumnary

Multi-line description.
"""

(except for test_ method docstrings, of course).

See previous comment about docstring formatting.

""" will fit on previous line.

See previous comment about docstring formatting

This attribution should be in the commit message, I believe. May want to ask Christian. Best to include the URL instead of just the ID.

Needs module-level docstring.

See comment re: formatting docstrings.

See comment re: formatting docstrings.

See comment re: formatting docstrings.

See comment re: formatting docstrings.

See comment re: formatting docstrings.

See comment re: formatting docstrings.

See comment re: formatting docstrings.

See comment re: formatting docstrings.

See comment re: formatting docstrings.

See comment re: formatting docstrings.

Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py



Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py

Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py

Blank line between these.

Docstring (just needs to indicate that its a dummy model for tests.)

Likewise.

Blank line between these.

Single quotes.

Single quotes.

Single quotes.

Single quotes.

Single quotes.

Single quotes.

Blank line between these.

Unused method.

Since this is a private method for tests, lets call it _validate_credentials.
Also, needs a docstring.

You can reflow this to fill more of the first line.

Single quotes.

This should be comparing against a constant.

Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py

Your description is missing a period.

No leading space. Missing a period.

Missing a period.

Missing super(...).tearDown()

You should just use the constant here.

Single quotes.

You want to use bytes(random()).
We avoid str because in Python 2 str is a binary type and not a text type. It has been renamed to bytes in Python 3 and backported. The text type of Python 2 is unicode (which was renamed to str in Python 3). For actual string types, use six.text_type which converts to unicode in py2 and str in py3. For binary data (such as HTTP headers) use bytes. (There is also six.binary_type, but since we are Python 2.7 only we can use bytes instead.)

bytes(random())

missing super(...).setUp()

Single quotes.

missing super(...).tearDown()

No leading or trailing spaces.

Missing a period.

Missing args, returns.

Use either:

        clean_credentials = \
            self.basic_auth_backend.clean_credentials_for_display(
                credentials)


or:

        clean_credentials = (
            self.basic_auth_backend
            .clean_credentials_for_display(
                credentials
            )
        )

Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py

Please re-write your summary in the imperitive mood.

You should inherit from SpyAgency e.g.

class WebAPITokenAuthBackendTests(SpyAgency, TestModelsLoaderMixin, TestCase):
    # ...


That way, you dont need self.agency, nor do you need your tearDown method -- SpyAgency.tearDown() (which will be called after each test) will unspy on all spies!

"succeeds"

Add a trailing , or move the ) onto this line.

Same here.

Since you're inheriting from SpyAgency, you don't need to unspy in tearDown -- it will happen automatically.

Can you rename this to TestWebAPITokenModel

TestTokenAuthBackend.

You will want to use assertIsNone(result). When comparing against None, you always want to use is or is not.

assertIsNone

You aren't using middleware or requestfactory in the testcases so why not:


self.request = RequestFactory().get('/')
SessionMiddleware().process_request(self.request)

Does the User need to exist in the database? If not we can just make User() object.

Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py



Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py

"web API"

missing period.

Can you move this into test_get_credentials_missing_credentials? It is only used in that test.

Unnecessary.

Put the trailing space on the previous line, even if it means splitting the string earlier.

You can move this to a single line statement -- you won't need the comma in that case.

Is there any particular reason we are doing rand() here instead of just using a non-matching token

Is there any particular reason we are doing rand() here instead of just using a non-matching token

Missing period.
web API

import foo statements go above from foo import bar statements.

Can you move this into test_get_credentials_malformed_credentials? It is only used in that test.

Unnecessary method. If this class doesn't implement tearDown, the superclass method will be invoked instead.

This sentence contains a comma splice. How about:

"""Fake version of validate credentials that always succeeds.

This is used in lieu of the actual method for testing purposes.

...
"""

or similar.

None.
Only one space between sentences.

"request"
Missing period.

"non-basic"

"with"

"with"

Missing trailing comma.

Missing trailing comma.

Missing trailing comma.

Missing trailing comma.

Missing trailing comma.

Missing trailing comma.

Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py

Please include in your summary:

This patch is based on work by Minh Le Hong at (the URL)

print() is not used.

Alphabetize.

Alphabetize imports.

Blank line between these.

print() is not used.

Alphabetize these.

Blank line between these.

Can you rearrange this so that things are defined just before they're tested?

result = self.api_token_auth_backend.get_credentials(self.request)
self.assertIsNone(result)

warning_message = logging.warning.spy.last_call.args[0]
self.assertTrue(warning_message.startswith(
    ...)

Please use single quotes around HTTP_AUTHORIZATION

username and password are only used the once, so we could bring them inline instead of creating variables:

self.user = User.objects.create_user(
    username='testuser', password='testuser_password')


That said, I don't see anywhere that we actually care about the password being set, so we could leave that out entirely.

Same comments about username/password.

username and password are only used once, so we can simplify:

encoded_credentials = 'testuser:testpassword'.encode('base64')

Same here.

Same here.

It's a little silly to define a credentials dict just to unpack it for kwargs. Let's change this to be:

result = self.basic_auth_backend.login_with_credentials(
    request, username=username, password=password)


and kill the credentials definition.

Same here re: credentials.

Same here re: credentials

Same here re: credentials

Same here re: credentials

Same here re: credentials

Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py

can also include that function returns None in function doc string. I believe like

Returns:
     None

Thanks stensby - just a few questions. See below.

Why is this 'invalidtoken123? This is a valid token, no? Kinda makes the test confusing when the invalidtoken is valid.

Same as above.

I don't think "always succeeds" is true. Because this is returning None, it means we're falling back to the base authentication mechanism. At least, that's what I understand from reading https://github.com/djblets/djblets/blob/c4fa360ad80e72bab4cca142e17a90ee9dbfb68a/djblets/webapi/auth/backends/base.py#L59

No blank line here (django and kgb are both third-party libraries from the point of view of the djblets package).

No blank line here.

Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py



Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py

So I believe you may be testing this wrongly after reading further into it.
Firstly you are setting the user on the request AND short circuiting the validate_credentials method. We shouldn't have to do either of these things.
Instead, you should create the user and store it as self.user and set self.request.user = AnonymousUser(). That way, the real validate_credentials will be called and return None.

Tool: PEP8 Style Checker
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py



Tool: Pyflakes
Processed Files:
    djblets/webapi/tests/test_basic_auth_backend.py
    djblets/webapi/tests/test_api_auth_backend.py

Ship It!