Add unit tests for djblets.log.views
Review Request #10833 — Created Jan. 18, 2020 and updated
Add a unit test for djblets.log.views.
Adding
parse_timestamp()
tests,def build_query_string()
tests,iter_log_lines()
tests, andget_log_filtersets()
tests.
- Added a unit test for djblets.log.views, which passes (along with all other tests except ERROR: Failure: ImproperlyConfigured (Error loading MySQLdb module: No module named MySQLdb)).
- Running
./tests/runtests.py djblets.log.tests
passes all tests.
Description | From | Last Updated |
---|---|---|
F401 'time' imported but unused |
reviewbot | |
F401 'logging' imported but unused |
reviewbot | |
F401 'importlib.import_module' imported but unused |
reviewbot | |
F401 'django.db.connection' imported but unused |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
F401 'django.contrib.auth.models.AnonymousUser' imported but unused |
reviewbot | |
F401 'djblets.log.views.server_log' imported but unused |
reviewbot | |
E302 expected 2 blank lines, found 1 |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E202 whitespace before ')' |
reviewbot | |
E501 line too long (80 > 79 characters) |
reviewbot | |
E501 line too long (119 > 79 characters) |
reviewbot | |
E202 whitespace before ')' |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E231 missing whitespace after ':' |
reviewbot | |
E501 line too long (82 > 79 characters) |
reviewbot | |
E501 line too long (92 > 79 characters) |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E501 line too long (99 > 79 characters) |
reviewbot | |
E225 missing whitespace around operator |
reviewbot | |
E501 line too long (93 > 79 characters) |
reviewbot | |
E501 line too long (89 > 79 characters) |
reviewbot | |
E501 line too long (90 > 79 characters) |
reviewbot | |
E501 line too long (90 > 79 characters) |
reviewbot | |
E501 line too long (92 > 79 characters) |
reviewbot | |
E501 line too long (91 > 79 characters) |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E302 expected 2 blank lines, found 1 |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E202 whitespace before ')' |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E202 whitespace before ')' |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E202 whitespace before ')' |
reviewbot | |
E202 whitespace before ')' |
reviewbot | |
E202 whitespace before ')' |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E202 whitespace before ')' |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E202 whitespace before ')' |
reviewbot | |
E202 whitespace before ')' |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E202 whitespace before ')' |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E225 missing whitespace around operator |
reviewbot | |
E502 the backslash is redundant between brackets |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E502 the backslash is redundant between brackets |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E302 expected 2 blank lines, found 1 |
reviewbot | |
E303 too many blank lines (2) |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E202 whitespace before ')' |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E203 whitespace before ',' |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E203 whitespace before ',' |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E127 continuation line over-indented for visual indent |
reviewbot | |
E302 expected 2 blank lines, found 1 |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E501 line too long (81 > 79 characters) |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E203 whitespace before ',' |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E203 whitespace before ',' |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E203 whitespace before ',' |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E203 whitespace before ',' |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E203 whitespace before ',' |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E303 too many blank lines (3) |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
Some important notes for the whole start-of-file area, including imports: Each file needs a docstring. Even unit test files. Something … |
chipx86 | |
While it's great practice to put docstrings in functions (and we highly encourage it), the exception is actually setUp() and … |
chipx86 | |
There's a ton of modules named "views," so the "views" prefix in here isn't really all that helpful. We tend … |
chipx86 | |
These variables can go inline in the call. Keeps the test nice and tidy, and makes it easier to copy/paste … |
chipx86 | |
When comparing against None, you'll want to use assertIsNone (or assertIsNotNone). |
chipx86 | |
We don't generally prefix with "mock," because at the end of the day it just becomes additional noise in the … |
chipx86 | |
A space snuck in at the end of this docstring. Same on some other ones. |
chipx86 | |
We want to always use single quotes instead of double quotes, unless the string itself contains single quotes (like "It's'a … |
chipx86 | |
We always like to expand dictionaries, so that it's easier to add to them without reformatting everything and easier to … |
chipx86 | |
You can nuke this blank line. Same with any others that introduce one. The only time we do want a … |
chipx86 | |
Good practice in Python when concatenating strings is to use a format string. It's faster than using +. So: '%s.log' … |
chipx86 | |
Here's some places we do want a blank line. Any time you are: Separating two blocks from each other (exception … |
chipx86 | |
Since the line_info[1] values are never going to change, you'll be better served with elif blocks. |
chipx86 | |
The u'...' string prefixes are old, and not something we want to use anymore. They imply a Unicode string, which … |
chipx86 | |
You'll want a blank line between these (separating a statement from a block). |
chipx86 | |
While commonly done in Python programs, it's actually dangerous for us to assign anything to _. That's because _ is … |
chipx86 | |
Looks like a blank line snuck in here. |
chipx86 | |
W291 trailing whitespace |
reviewbot | |
E123 closing bracket does not match indentation of opening bracket's line |
reviewbot | |
E123 closing bracket does not match indentation of opening bracket's line |
reviewbot | |
E126 continuation line over-indented for hanging indent |
reviewbot | |
E126 continuation line over-indented for hanging indent |
reviewbot | |
E126 continuation line over-indented for hanging indent |
reviewbot | |
E126 continuation line over-indented for hanging indent |
reviewbot | |
unicode(name) doesn't work in Python 3 because the unicode function got removed. Do we need the unicode thing at all? … |
mike_conley |
- Commit:
-
b77030c384038039c44de6b93c61d4c5bdddab2c8443d35ba5fe0f1a9b60b17cff49449ee4739eea
Checks run (1 failed, 1 succeeded)
flake8
-
Warning: Showing 30 of 216 failures.
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Commit:
-
8443d35ba5fe0f1a9b60b17cff49449ee4739eea735d9f71352287a4a857d7e63159d0b19a5434ff
Checks run (1 failed, 1 succeeded)
flake8
-
Warning: Showing 30 of 146 failures.
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Commit:
-
735d9f71352287a4a857d7e63159d0b19a5434ff22bfd68832391f39290e5a0074cbc9757e60baeb
Checks run (1 failed, 1 succeeded)
flake8
-
Warning: Showing 30 of 87 failures.
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Bugs:
-
- Commit:
d10b356249d244d6988589c4ae20c97ee48ecb6d3d3eeeaf89212f8ad505a0ef9887d870c0221c36Checks run (2 succeeded)
flake8 passed.JSHint passed.
-
This is a great batch of new unit tests. Should definitely help us catch regressions in the future.
There's some stuff I'll have you fix up. Mostly having to do with styles and standards, simplifying the number of lines of code in some tests, some gotchas you wouldn't otherwise know to look for, etc., but the core test logic looks sound.
-
Some important notes for the whole start-of-file area, including imports:
- Each file needs a docstring. Even unit test files. Something like
"""Unit tests for djblets.log.views."""
is fine (it's not uncommon for this to match the class's docstring in these cases). - The very first import must be
from __future__ import unicode_literals
, since that tells Python we want string compatibility between Python 2 and 3 (makes all strings Unicode by default). This avoids a LOT of problems. - There's a bunch of conventions for imports that must be followed. See the coding style guide for information on this.
- Each file needs a docstring. Even unit test files. Something like
-
While it's great practice to put docstrings in functions (and we highly encourage it), the exception is actually
setUp()
andtearDown()
in unit tests.The reason basically just boils down to these functions being just part of the test infrastructure, and are so common that a docstring doesn't add anything. We're not generating codebase docs from them. We're not educating anyone with them. We'd just have a thousand variations on "Setting up for the test." So we don't really need to bother.
-
There's a ton of modules named "views," so the "views" prefix in here isn't really all that helpful. We tend not to put module names in docstrings. You can instead just say "Testing parse_timestamp ...."
This applies to the other ones below.
-
These variables can go inline in the call. Keeps the test nice and tidy, and makes it easier to copy/paste these into a shell or something if we need to manually duplicate behavior.
This will apply to a bunch of others below. Basically, any values that are only used once should just go inline in the call.
-
-
We don't generally prefix with "mock," because at the end of the day it just becomes additional noise in the test. Most values are going to be mock/fake/dummy/test data, so we can just assume that to be the case.
While you'll want to put these values directly into the call instead, if you were to pull them out (some later ones make sense since they're reused), I'd go with
timestamp_format
andtimestamp_str
. -
-
We want to always use single quotes instead of double quotes, unless the string itself contains single quotes (like
"It's'a me, Mario!"
). -
We always like to expand dictionaries, so that it's easier to add to them without reformatting everything and easier to scan for all the keys/values.
Though that said, like above, let's just make these inline:
self.assertEqual( build_query_string(request, { 'hello': 'world', }), '?hello=world')
-
You can nuke this blank line. Same with any others that introduce one.
The only time we do want a blank line after a docstring is for classes. I know, that sounds inconsistent, but it's a Python standard.
-
Good practice in Python when concatenating strings is to use a format string. It's faster than using
+
. So:'%s.log' % settings.LOGGING_NAME
These apply to other tests as well.
-
Here's some places we do want a blank line. Any time you are:
- Separating two blocks from each other (exception being something like
if/elif/else
ortry/except/finally
) - Separating statement from a block (like a variable followed by an
if
orfor
) - Separating a block from a statement
Think of each
for
loop as its own paragraph. It'll make it easier to read. - Separating two blocks from each other (exception being something like
-
-
The
u'...'
string prefixes are old, and not something we want to use anymore. They imply a Unicode string, which is the default on Python 3 and the default for any Python 2 files that dofrom __future__ import unicode_literals
(which should be every single Python file we write). -
-
While commonly done in Python programs, it's actually dangerous for us to assign anything to
_
. That's because_
is used as an alias forugettext
, which is for string translations. It could potentially be overridden if someone's not careful, and it's also scanned by the program that generates the string translation files.Instead, just tack a
[1]
on the end of the statement or, maybe more correctly, name this value and then test it for correctness. -
- Commit:
-
3d3eeeaf89212f8ad505a0ef9887d870c0221c362e7340ef24aa1eda0f070dcb0941400729180b62
- Diff:
-
Revision 7 (+382)
Checks run (1 failed, 1 succeeded)
flake8
- Change Summary:
-
make changes accrording to Christian's comments.
Major changes:
1. editimport
format
2. change "" to ''
3. expand dictionaries
4. take variables go inline in the call
5. littles changes based on Christian's comments - Commit:
-
2e7340ef24aa1eda0f070dcb0941400729180b62e88ef3ff6171c8df91636705916c36a3d1cc06a0
- Diff:
-
Revision 8 (+382)
Checks run (2 succeeded)
-
Thanks, Xiaole! Nice tests. Just one thing from what I can tell - see below.
-
unicode(name)
doesn't work in Python 3 because theunicode
function got removed.Do we need the
unicode
thing at all? What happens if we just use:self.assertEqual(name, 'By date')
?
Same goes for the other places where you use
unicode()
in this file.
- Change Summary:
-
removed
unicode()
and all tests pass - Commit:
-
e88ef3ff6171c8df91636705916c36a3d1cc06a02b03f4b44e507ce16e81cceeff03ae2e4d76049a
- Diff:
-
Revision 9 (+382)