Summary

Add unit tests for Djblets web API backend auth.

Review Request #8440 — Created Sept. 25, 2016 and submitted Nov. 26, 2016, 5:05 p.m.

Information

Owner

stensby

Repository

Djblets

Branch

master

Bugs

Depends On

Commit

6542556...

Reviewers

Groups

djblets, students

People

Description

Added unit tests for WebAPITokenAuthBackend and WebAPIBasicAuthBackend.

This patch is based on previous work by Minh Le Hoang at
https://reviews.reviewboard.org/r/7997/.

Testing Done

All tests pass.

Issues

Description	From	Last Updated
Your description is missing a period.	brennie	Oct. 20, 2016, 7:43 p.m.
Please re-write your summary in the imperitive mood.	brennie	Oct. 23, 2016, 11:50 a.m.
Please include in your summary: This patch is based on work by Minh Le Hong at (the URL)	brennie	Oct. 29, 2016, 8:35 p.m.
local variable 'user' is assigned to but never used	reviewbot	Oct. 14, 2016, 2:38 p.m.
Col: 41 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 14, 2016, 2:38 p.m.
local variable 'get_credentials_mock' is assigned to but never used	reviewbot	Oct. 14, 2016, 2:38 p.m.
local variable 'get_credentials_mock' is assigned to but never used	reviewbot	Oct. 14, 2016, 2:38 p.m.
Col: 53 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 14, 2016, 2:38 p.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Oct. 14, 2016, 2:38 p.m.
Col: 41 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 14, 2016, 2:38 p.m.
Col: 41 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 14, 2016, 2:38 p.m.
Col: 41 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 14, 2016, 2:38 p.m.
You don't need the entire module path in here, you can just do: """Unit tests for the WebAPITokenBackend.""" For future …	brennie	Oct. 16, 2016, 11:45 a.m.
See previous comment about docstring formatting.	brennie	Oct. 16, 2016, 12:19 p.m.
""" will fit on previous line.	brennie	Oct. 16, 2016, 11:46 a.m.
See previous comment about docstring formatting	brennie	Oct. 16, 2016, 11:46 a.m.
This attribution should be in the commit message, I believe. May want to ask Christian. Best to include the URL …	brennie	Oct. 16, 2016, 11:46 a.m.
Needs module-level docstring.	brennie	Oct. 17, 2016, 4:53 p.m.
See comment re: formatting docstrings.	brennie	Oct. 16, 2016, 11:46 a.m.
Col: 53 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 14, 2016, 2:52 p.m.
See comment re: formatting docstrings.	brennie	Oct. 16, 2016, 11:46 a.m.
See comment re: formatting docstrings.	brennie	Oct. 16, 2016, 11:46 a.m.
See comment re: formatting docstrings.	brennie	Oct. 16, 2016, 11:46 a.m.
See comment re: formatting docstrings.	brennie	Oct. 16, 2016, 11:46 a.m.
Col: 41 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 14, 2016, 2:53 p.m.
See comment re: formatting docstrings.	brennie	Oct. 16, 2016, 11:46 a.m.
Col: 41 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 14, 2016, 2:53 p.m.
See comment re: formatting docstrings.	brennie	Oct. 16, 2016, 11:46 a.m.
Col: 41 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 14, 2016, 2:53 p.m.
See comment re: formatting docstrings.	brennie	Oct. 16, 2016, 11:46 a.m.
Col: 41 E126 continuation line over-indented for hanging indent	reviewbot	Oct. 14, 2016, 2:54 p.m.
See comment re: formatting docstrings.	brennie	Oct. 16, 2016, 11:46 a.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Oct. 14, 2016, 2:54 p.m.
See comment re: formatting docstrings.	brennie	Oct. 16, 2016, 11:46 a.m.
Col: 13 E128 continuation line under-indented for visual indent	reviewbot	Oct. 14, 2016, 2:54 p.m.
Blank line between these.	brennie	Oct. 19, 2016, 6:42 p.m.
Docstring (just needs to indicate that its a dummy model for tests.)	brennie	Oct. 19, 2016, 7:05 p.m.
Likewise.	brennie	Oct. 19, 2016, 7:05 p.m.
Blank line between these.	brennie	Oct. 19, 2016, 7:07 p.m.
Single quotes.	brennie	Oct. 19, 2016, 7:05 p.m.
Single quotes.	brennie	Oct. 19, 2016, 7:05 p.m.
Single quotes.	brennie	Oct. 19, 2016, 7:05 p.m.
Single quotes.	brennie	Oct. 19, 2016, 7:05 p.m.
Single quotes.	brennie	Oct. 19, 2016, 7:06 p.m.
Single quotes.	brennie	Oct. 19, 2016, 7:06 p.m.
Blank line between these.	brennie	Oct. 19, 2016, 6:42 p.m.
Unused method.	brennie	Oct. 19, 2016, 6:59 p.m.
Since this is a private method for tests, lets call it _validate_credentials. Also, needs a docstring.	brennie	Oct. 19, 2016, 6:59 p.m.
You can reflow this to fill more of the first line.	brennie	Oct. 19, 2016, 7:07 p.m.
Single quotes.	brennie	Oct. 19, 2016, 7:02 p.m.
This should be comparing against a constant.	brennie	Oct. 19, 2016, 8:30 p.m.
No leading space. Missing a period.	brennie	Oct. 20, 2016, 7:43 p.m.
Missing a period.	brennie	Oct. 20, 2016, 7:43 p.m.
Missing super(...).tearDown()	brennie	Oct. 20, 2016, 7:43 p.m.
You should just use the constant here.	brennie	Oct. 20, 2016, 7:43 p.m.
Single quotes.	brennie	Oct. 20, 2016, 7:42 p.m.
You want to use bytes(random()). We avoid str because in Python 2 str is a binary type and not a …	brennie	Oct. 20, 2016, 7:42 p.m.
bytes(random())	brennie	Oct. 20, 2016, 7:42 p.m.
missing super(...).setUp()	brennie	Oct. 20, 2016, 7:42 p.m.
Single quotes.	brennie	Oct. 20, 2016, 7:42 p.m.
missing super(...).tearDown()	brennie	Oct. 20, 2016, 7:42 p.m.
No leading or trailing spaces. Missing a period. Missing args, returns.	brennie	Oct. 20, 2016, 8:23 p.m.
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 ) )	brennie	Oct. 20, 2016, 7:42 p.m.
You should inherit from SpyAgency e.g. class WebAPITokenAuthBackendTests(SpyAgency, TestModelsLoaderMixin, TestCase): # ... That way, you dont need self.agency, nor do …	brennie	Oct. 23, 2016, 11:47 a.m.
"succeeds"	brennie	Oct. 23, 2016, 11:46 a.m.
Add a trailing , or move the ) onto this line.	brennie	Oct. 23, 2016, 11:46 a.m.
Same here.	brennie	Oct. 23, 2016, 11:46 a.m.
Since you're inheriting from SpyAgency, you don't need to unspy in tearDown -- it will happen automatically.	brennie	Oct. 23, 2016, 11:47 a.m.
Can you rename this to TestWebAPITokenModel	brennie	Oct. 23, 2016, 2:44 p.m.
TestTokenAuthBackend.	brennie	Oct. 23, 2016, 2:44 p.m.
You will want to use assertIsNone(result). When comparing against None, you always want to use is or is not.	brennie	Oct. 23, 2016, 2:45 p.m.
assertIsNone	brennie	Oct. 23, 2016, 2:45 p.m.
You aren't using middleware or requestfactory in the testcases so why not: self.request = RequestFactory().get('/') SessionMiddleware().process_request(self.request)	brennie	Oct. 23, 2016, 2:53 p.m.
Does the User need to exist in the database? If not we can just make User() object.	brennie	Oct. 26, 2016, 2:27 p.m.
"web API" missing period.	brennie	Oct. 28, 2016, 2:16 p.m.
Can you move this into test_get_credentials_missing_credentials? It is only used in that test.	brennie	Oct. 28, 2016, 2:17 p.m.
Unnecessary.	brennie	Oct. 28, 2016, 2:17 p.m.
Put the trailing space on the previous line, even if it means splitting the string earlier.	brennie	Oct. 28, 2016, 2:19 p.m.
You can move this to a single line statement -- you won't need the comma in that case.	brennie	Oct. 28, 2016, 2:19 p.m.
Is there any particular reason we are doing rand() here instead of just using a non-matching token	brennie	Oct. 28, 2016, 2:20 p.m.
Is there any particular reason we are doing rand() here instead of just using a non-matching token	brennie	Oct. 28, 2016, 2:20 p.m.
Missing period. web API	brennie	Oct. 28, 2016, 2:21 p.m.
import foo statements go above from foo import bar statements.	brennie	Oct. 28, 2016, 2:21 p.m.
Can you move this into test_get_credentials_malformed_credentials? It is only used in that test.	brennie	Oct. 28, 2016, 2:21 p.m.
Unnecessary method. If this class doesn't implement tearDown, the superclass method will be invoked instead.	brennie	Oct. 28, 2016, 2:21 p.m.
This sentence contains a comma splice. How about: """Fake version of validate credentials that always succeeds. This is used in …	brennie	Oct. 28, 2016, 2:23 p.m.
None. Only one space between sentences.	brennie	Oct. 28, 2016, 2:23 p.m.
"request" Missing period.	brennie	Oct. 28, 2016, 2:29 p.m.
"non-basic"	brennie	Oct. 28, 2016, 2:27 p.m.
"with"	brennie	Oct. 28, 2016, 2:27 p.m.
"with"	brennie	Oct. 28, 2016, 2:27 p.m.
Missing trailing comma.	brennie	Oct. 28, 2016, 2:28 p.m.
Missing trailing comma.	brennie	Oct. 28, 2016, 2:28 p.m.
Missing trailing comma.	brennie	Oct. 28, 2016, 2:28 p.m.
Missing trailing comma.	brennie	Oct. 28, 2016, 2:28 p.m.
Missing trailing comma.	brennie	Oct. 28, 2016, 2:28 p.m.
Missing trailing comma.	brennie	Oct. 28, 2016, 2:28 p.m.
print() is not used.	brennie	Oct. 28, 2016, 5:28 p.m.
Alphabetize.	brennie	Oct. 28, 2016, 5:32 p.m.
Alphabetize imports.	brennie	Oct. 28, 2016, 5:32 p.m.
Blank line between these.	brennie	Oct. 28, 2016, 5:30 p.m.
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] …	david	Oct. 29, 2016, 8:22 p.m.
Please use single quotes around HTTP_AUTHORIZATION	david	Oct. 28, 2016, 5:39 p.m.
username and password are only used the once, so we could bring them inline instead of creating variables: self.user = …	david	Oct. 28, 2016, 6:02 p.m.
Same comments about username/password.	david	Oct. 28, 2016, 6:03 p.m.
print() is not used.	brennie	Oct. 28, 2016, 5:30 p.m.
Alphabetize these.	brennie	Oct. 28, 2016, 5:32 p.m.
Blank line between these.	brennie	Oct. 28, 2016, 5:32 p.m.
can also include that function returns None in function doc string. I believe like Returns: None	LA larmiej	Oct. 29, 2016, 8:43 p.m.
username and password are only used once, so we can simplify: encoded_credentials = 'testuser:testpassword'.encode('base64')	david	Oct. 28, 2016, 6:09 p.m.
Same here.	david	Oct. 28, 2016, 6:09 p.m.
Same here.	david	Oct. 28, 2016, 6:10 p.m.
It's a little silly to define a credentials dict just to unpack it for kwargs. Let's change this to be: …	david	Oct. 28, 2016, 6:13 p.m.
Same here re: credentials.	david	Oct. 28, 2016, 6:13 p.m.
Same here re: credentials	david	Oct. 28, 2016, 6:16 p.m.
Same here re: credentials	david	Oct. 28, 2016, 6:28 p.m.
Same here re: credentials	david	Oct. 28, 2016, 6:29 p.m.
Same here re: credentials	david	Oct. 28, 2016, 6:35 p.m.
No blank line here (django and kgb are both third-party libraries from the point of view of the djblets package).	david	Nov. 9, 2016, 6:11 p.m.
Why is this 'invalidtoken123? This is a valid token, no? Kinda makes the test confusing when the invalidtoken is valid.	mike_conley	Nov. 8, 2016, 6:48 p.m.
Same as above.	mike_conley	Nov. 8, 2016, 6:48 p.m.
No blank line here.	david	Nov. 9, 2016, 6:11 p.m.
So I believe you may be testing this wrongly after reading further into it. Firstly you are setting the user …	brennie	Nov. 10, 2016, 4:40 p.m.
I don't think "always succeeds" is true. Because this is returning None, it means we're falling back to the base …	mike_conley	Nov. 8, 2016, 6:48 p.m.

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

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 1)
Show all issues
```
 local variable 'user' is assigned to but never used
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 1)
Show all issues
```
Col: 41
 E126 continuation line over-indented for hanging indent
```

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 1)

Show all issues

 local variable 'get_credentials_mock' is assigned to but never used

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 1)

Show all issues

 local variable 'get_credentials_mock' is assigned to but never used

Commit:

-	45d7073e448733130e7781c3dbd53d9407397854
+	94e1bd99317a2e155227ae2f88f693acce0b5778

Diff:

Revision 2 (+217)

Show changes

djblets/webapi/tests/test_basic_auth_backend.py

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

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 2)
Show all issues
```
Col: 53
 E126 continuation line over-indented for hanging indent
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 2)
Show all issues
```
Col: 9
 E265 block comment should start with '# '
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 2)
Show all issues
```
Col: 41
 E126 continuation line over-indented for hanging indent
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 2)
Show all issues
```
Col: 41
 E126 continuation line over-indented for hanging indent
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 2)
Show all issues
```
Col: 41
 E126 continuation line over-indented for hanging indent
```

Summary:

-	[WIP] Adding unit tests for djblets.webapi.auth.backends.basic
+	Adding unit tests for djblets.webapi.auth.backends.basic

Description:

~		[WIP] Adding unit tests for djblets.webapi.auth.backends.basic
	~	Adding unit tests for djblets.webapi.auth.backends.basic

Testing Done:

All tests pass

Commit:

-	94e1bd99317a2e155227ae2f88f693acce0b5778
+	c5b80cec4ae11baccab7db8a269f51e6dc9d71d0

Diff:

Revision 3 (+367)

Show changes

	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_basic_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

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
Col: 53
 E126 continuation line over-indented for hanging indent
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
Col: 41
 E126 continuation line over-indented for hanging indent
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
Col: 41
 E126 continuation line over-indented for hanging indent
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
Col: 41
 E126 continuation line over-indented for hanging indent
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
Col: 41
 E126 continuation line over-indented for hanging indent
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
Col: 13
 E128 continuation line under-indented for visual indent
```

Change Summary:

Indentation fixes

Commit:

-	c5b80cec4ae11baccab7db8a269f51e6dc9d71d0
+	b3a7397c0d4d5fded8a9e3cb6a9f545f9f7d7198

Diff:

Revision 4 (+367)

Show changes

	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_basic_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

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 3)

Show all issues

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).

stensby Oct. 14, 2016, 3:09 p.m.

Sorry about that! I copied docstrings from Minh's RR.

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 3)

Show all issues

See previous comment about docstring formatting.

stensby Oct. 16, 2016, 11:21 a.m.

For all of the test docstrings would you prefer the following format?

"""Testing Token Auth get_credentials with missing token."""

david Oct. 16, 2016, 11:32 a.m.

Yes, with the exception that we don't include the trailing period because the test suite will add "..." in the output.

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 3)
Show all issues
```
""" will fit on previous line.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 3)
Show all issues
```
See previous comment about docstring formatting
```

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 3)

Show all issues

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.

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
Needs module-level docstring.
```
1. ST
  
  stensby Oct. 16, 2016, 11:47 a.m.
  What do you mean by module level docstring?
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
See comment re: formatting docstrings.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
See comment re: formatting docstrings.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
See comment re: formatting docstrings.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
See comment re: formatting docstrings.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
See comment re: formatting docstrings.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
See comment re: formatting docstrings.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
See comment re: formatting docstrings.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
See comment re: formatting docstrings.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
See comment re: formatting docstrings.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 3)
Show all issues
```
See comment re: formatting docstrings.
```

Summary:

-	Adding unit tests for djblets.webapi.auth.backends.basic
+	Djblets webapi backend auth tests

Description:

~		Adding unit tests for djblets.webapi.auth.backends.basic
	~	Added unit tests for WebAPITokenAuthBackend and WebAPIBasicAuthBackend

Commit:

-	b3a7397c0d4d5fded8a9e3cb6a9f545f9f7d7198
+	c14d727af92fa6ca060f5d3d30f4ce4ab4e5b534

Diff:

Revision 5 (+345)

Show changes

	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_basic_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

Commit:

-	c14d727af92fa6ca060f5d3d30f4ce4ab4e5b534
+	e9072816ca992db1cec4a93b48bba9a1e03ff01e

Diff:

Revision 6 (+347)

Show changes

	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_basic_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

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 6)
Show all issues
```
Blank line between these.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 6)
Show all issues
```
Docstring (just needs to indicate that its a dummy model for tests.)
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 6)
Show all issues
```
Likewise.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 6)
Show all issues
```
Blank line between these.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 6)
Show all issues
```
Single quotes.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 6)
Show all issues
```
Single quotes.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 6)
Show all issues
```
Single quotes.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 6)
Show all issues
```
Single quotes.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 6)
Show all issues
```
Single quotes.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 6)
Show all issues
```
Single quotes.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 6)
Show all issues
```
Blank line between these.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 6)
Show all issues
```
Unused method.
```

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 6)

Show all issues

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

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 6)
Show all issues
```
You can reflow this to fill more of the first line.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 6)
Show all issues
```
Single quotes.
```

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 6)

Show all issues

This should be comparing against a constant.

stensby Oct. 19, 2016, 7:08 p.m.

Do you mean I should use a single assert statement against the entire dictionary?

Change Summary:

Style fixes

Commit:

-	e9072816ca992db1cec4a93b48bba9a1e03ff01e
+	2952cd71cbd0f837be4abe7dd0d45727df374c1d

Diff:

Revision 7 (+351)

Show changes

	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_basic_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

Show all issues
```
Your description is missing a period.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 7)
Show all issues
```
No leading space. Missing a period.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 7)
Show all issues
```
Missing a period.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 7)
Show all issues
```
Missing super(...).tearDown()
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 7)
Show all issues
```
You should just use the constant here.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 7)
Show all issues
```
Single quotes.
```

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 7)

Show all issues

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.)

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 7)
Show all issues
```
bytes(random())
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 7)
Show all issues
```
missing super(...).setUp()
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 7)
Show all issues
```
Single quotes.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 7)
Show all issues
```
missing super(...).tearDown()
```

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 7)

Show all issues

No leading or trailing spaces.
Missing a period.
Missing args, returns.

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 7)

Show all issues

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
            )
        )

Description:

~		Added unit tests for WebAPITokenAuthBackend and WebAPIBasicAuthBackend
	~	Added unit tests for WebAPITokenAuthBackend and WebAPIBasicAuthBackend.

Testing Done:

~		All tests pass
	~	All tests pass.

Commit:

-	2952cd71cbd0f837be4abe7dd0d45727df374c1d
+	796c3d875e87b6e7b4bd5ca5bb9a785b6a8d2d10

Diff:

Revision 8 (+367)

Show changes

	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_basic_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

Show all issues

Please re-write your summary in the imperitive mood.

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 8)

Show all issues

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!

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 8)
Show all issues
```
"succeeds"
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 8)
Show all issues
```
Add a trailing , or move the ) onto this line.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 8)
Show all issues
```
Same here.
```

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 8)

Show all issues

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

Summary:

-	Djblets webapi backend auth tests
+	Add unit tests for Djblets webapi backend auth

Commit:

-	796c3d875e87b6e7b4bd5ca5bb9a785b6a8d2d10
+	7bb5dd57c5c8bc054e16c91f24acd1e6c8b15ac9

Diff:

Revision 9 (+361)

Show changes

	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_basic_auth_backend.py

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 9)
Show all issues
```
Can you rename this to TestWebAPITokenModel
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 9)
Show all issues
```
TestTokenAuthBackend.
```

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 9)

Show all issues

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

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 9)
Show all issues
```
assertIsNone
```

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 9)

Show all issues

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


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

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 9)

Show all issues

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

stensby Oct. 23, 2016, 2:53 p.m.

Two of the functions being tested end up doing a db query for that user to check credentials. Is it a problem that it creates a user? I was under the impression that a test database is created and then destroyed.

brennie Oct. 26, 2016, 2:27 p.m.
```
No, its no problem. I was just curious.
```

Commit:

-	7bb5dd57c5c8bc054e16c91f24acd1e6c8b15ac9
+	dde3462c7a94f5f0df6380d9f1df28caae105058

Diff:

Revision 10 (+355)

Show changes

	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_basic_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



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

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 10)
Show all issues
```
"web API"

missing period.
```

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 10)

Show all issues

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

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 10)
Show all issues
```
Unnecessary.
```

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 10)

Show all issues

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

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 10)

Show all issues

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

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 10)

Show all issues

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

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 10)

Show all issues

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

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)
Show all issues
```
Missing period.
web API
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)
Show all issues
```
import foo statements go above from foo import bar statements.
```

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)

Show all issues

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

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)

Show all issues

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

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)

Show all issues

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.

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)
Show all issues
```
None.
Only one space between sentences.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)
Show all issues
```
"request"
Missing period.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)
Show all issues
```
"non-basic"
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)
Show all issues
```
"with"
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)
Show all issues
```
"with"
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)
Show all issues
```
Missing trailing comma.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)
Show all issues
```
Missing trailing comma.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)
Show all issues
```
Missing trailing comma.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)
Show all issues
```
Missing trailing comma.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)
Show all issues
```
Missing trailing comma.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 10)
Show all issues
```
Missing trailing comma.
```

Summary:

-	Add unit tests for Djblets webapi backend auth
+	Add unit tests for Djblets web API backend auth

Commit:

-	dde3462c7a94f5f0df6380d9f1df28caae105058
+	65e6c0ddcaa08ef2bca42ef790744319c932910e

Diff:

Revision 11 (+342)

Show changes

	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_basic_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

Summary:

-	Add unit tests for Djblets web API backend auth
+	Add unit tests for Djblets web API backend auth.

Show all issues

Please include in your summary:

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

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 11)
Show all issues
```
print() is not used.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 11)
Show all issues
```
Alphabetize.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 11)
Show all issues
```
Alphabetize imports.
```
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 11)
Show all issues
```
Blank line between these.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 11)
Show all issues
```
print() is not used.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 11)
Show all issues
```
Alphabetize these.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 11)
Show all issues
```
Blank line between these.
```

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 11)

Show all issues

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(
    ...)

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 11)
Show all issues
```
Please use single quotes around HTTP_AUTHORIZATION
```

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 11)

Show all issues

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.

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 11)
Show all issues
```
Same comments about username/password.
```

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 11)

Show all issues

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

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

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 11)
Show all issues
```
Same here.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 11)
Show all issues
```
Same here.
```

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 11)

Show all issues

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.

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 11)
Show all issues
```
Same here re: credentials.
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 11)
Show all issues
```
Same here re: credentials
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 11)
Show all issues
```
Same here re: credentials
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 11)
Show all issues
```
Same here re: credentials
```
djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 11)
Show all issues
```
Same here re: credentials
```

Description:

		Added unit tests for WebAPITokenAuthBackend and WebAPIBasicAuthBackend.
	+
	+	This patch is based on previous work by Minh Le Hoang at
	+	https://reviews.reviewboard.org/r/7997/.

Commit:

-	65e6c0ddcaa08ef2bca42ef790744319c932910e
+	575f51ff6b9d78053400b9805c1e00b69ae5d758

Diff:

Revision 12 (+304)

Show changes

	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_basic_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

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 11)

Show all issues

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

Returns:
     None

stensby Oct. 29, 2016, 8:44 p.m.

See: https://reviewboard.slack.com/archives/general/p1477019907002766

Ship it!

Thanks stensby - just a few questions. See below.

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 12)

Show all issues

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

stensby Nov. 3, 2016, 8:48 a.m.

What the token itself is doesn't really matter, it is named that to signify that it is not a token that has been handed to a user (so the token doesn't exist in our TestWebAPITokenModel). Any suggestions on what to rename it to so that is clear?

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 12)
Show all issues
```
Same as above.
```

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 12)

Show all issues

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

stensby Nov. 3, 2016, 8:48 a.m.

The function is actually replacing a function that is used to 'short-circut' the authentication process if the current user is already logged in.  See https://github.com/djblets/djblets/blob/c4fa360ad80e72bab4cca142e17a90ee9dbfb68a/djblets/webapi/auth/backends/base.py#L159

djblets/webapi/tests/test_api_auth_backend.py (Diff revision 12)

Show all issues

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

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 12)
Show all issues
```
No blank line here.
```

Commit:

-	575f51ff6b9d78053400b9805c1e00b69ae5d758
+	ccb73279c8210280766f31cf6e7b5ca93d66da51

Diff:

Revision 13 (+302)

Show changes

	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_basic_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

djblets/webapi/tests/test_basic_auth_backend.py (Diff revision 12)

Show all issues

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.

Change Summary:

Changed the request user to be anonymous, added additional test case for validate_credentials where the user is invalid.

Commit:

-	ccb73279c8210280766f31cf6e7b5ca93d66da51
+	65425560d3dcdc8b020bbf09f75b2c0f1128cc69

Diff:

Revision 14 (+284)

Show changes

	djblets/webapi/tests/test_api_auth_backend.py
	djblets/webapi/tests/test_basic_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

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.9.x (9b44865)