Add unit tests for djblets.log.views

Review Request #10833 — Created Jan. 18, 2020 and updated

Information

Djblets
release-1.0.x
2b03f4b...

Reviewers

Add a unit test for djblets.log.views.

Adding parse_timestamp() tests, def build_query_string() tests, iter_log_lines() tests, and get_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

reviewbotreviewbot

F401 'logging' imported but unused

reviewbotreviewbot

F401 'importlib.import_module' imported but unused

reviewbotreviewbot

F401 'django.db.connection' imported but unused

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

F401 'django.contrib.auth.models.AnonymousUser' imported but unused

reviewbotreviewbot

F401 'djblets.log.views.server_log' imported but unused

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E202 whitespace before ')'

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (119 > 79 characters)

reviewbotreviewbot

E202 whitespace before ')'

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E231 missing whitespace after ':'

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (92 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (99 > 79 characters)

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

E501 line too long (93 > 79 characters)

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E501 line too long (92 > 79 characters)

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E202 whitespace before ')'

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E202 whitespace before ')'

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E202 whitespace before ')'

reviewbotreviewbot

E202 whitespace before ')'

reviewbotreviewbot

E202 whitespace before ')'

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E202 whitespace before ')'

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E202 whitespace before ')'

reviewbotreviewbot

E202 whitespace before ')'

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E202 whitespace before ')'

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

E502 the backslash is redundant between brackets

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E502 the backslash is redundant between brackets

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E202 whitespace before ')'

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E203 whitespace before ','

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E203 whitespace before ','

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E203 whitespace before ','

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E203 whitespace before ','

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E203 whitespace before ','

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E203 whitespace before ','

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E203 whitespace before ','

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E303 too many blank lines (3)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

Some important notes for the whole start-of-file area, including imports: Each file needs a docstring. Even unit test files. Something …

chipx86chipx86

While it's great practice to put docstrings in functions (and we highly encourage it), the exception is actually setUp() and …

chipx86chipx86

There's a ton of modules named "views," so the "views" prefix in here isn't really all that helpful. We tend …

chipx86chipx86

These variables can go inline in the call. Keeps the test nice and tidy, and makes it easier to copy/paste …

chipx86chipx86

When comparing against None, you'll want to use assertIsNone (or assertIsNotNone).

chipx86chipx86

We don't generally prefix with "mock," because at the end of the day it just becomes additional noise in the …

chipx86chipx86

A space snuck in at the end of this docstring. Same on some other ones.

chipx86chipx86

We want to always use single quotes instead of double quotes, unless the string itself contains single quotes (like "It's'a …

chipx86chipx86

We always like to expand dictionaries, so that it's easier to add to them without reformatting everything and easier to …

chipx86chipx86

You can nuke this blank line. Same with any others that introduce one. The only time we do want a …

chipx86chipx86

Good practice in Python when concatenating strings is to use a format string. It's faster than using +. So: '%s.log' …

chipx86chipx86

Here's some places we do want a blank line. Any time you are: Separating two blocks from each other (exception …

chipx86chipx86

Since the line_info[1] values are never going to change, you'll be better served with elif blocks.

chipx86chipx86

The u'...' string prefixes are old, and not something we want to use anymore. They imply a Unicode string, which …

chipx86chipx86

You'll want a blank line between these (separating a statement from a block).

chipx86chipx86

While commonly done in Python programs, it's actually dangerous for us to assign anything to _. That's because _ is …

chipx86chipx86

Looks like a blank line snuck in here.

chipx86chipx86

W291 trailing whitespace

reviewbotreviewbot

E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

unicode(name) doesn't work in Python 3 because the unicode function got removed. Do we need the unicode thing at all? …

mike_conleymike_conley
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

xiaole2
Review request changed
Commit:
b77030c384038039c44de6b93c61d4c5bdddab2c
8443d35ba5fe0f1a9b60b17cff49449ee4739eea

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

xiaole2
Review request changed
Commit:
8443d35ba5fe0f1a9b60b17cff49449ee4739eea
735d9f71352287a4a857d7e63159d0b19a5434ff

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

xiaole2
Review request changed
Commit:
735d9f71352287a4a857d7e63159d0b19a5434ff
22bfd68832391f39290e5a0074cbc9757e60baeb

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

xiaole2
Review request changed
Commit:
22bfd68832391f39290e5a0074cbc9757e60baeb
d10b356249d244d6988589c4ae20c97ee48ecb6d

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

xiaole2
chipx86
  1. 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.

  2. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Some important notes for the whole start-of-file area, including imports:

    1. 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).
    2. 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.
    3. There's a bunch of conventions for imports that must be followed. See the coding style guide for information on this.
  3. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
     
    Show all issues

    While it's great practice to put docstrings in functions (and we highly encourage it), the exception is actually setUp() and tearDown() 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.

  4. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
    Show all issues

    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.

  5. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    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.

  6. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
     
    Show all issues

    When comparing against None, you'll want to use assertIsNone (or assertIsNotNone).

  7. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
     
    Show all issues

    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 and timestamp_str.

  8. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
    Show all issues

    A space snuck in at the end of this docstring. Same on some other ones.

  9. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
    Show all issues

    We want to always use single quotes instead of double quotes, unless the string itself contains single quotes (like "It's'a me, Mario!").

  10. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
     
    Show all issues

    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')
    
  11. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
     
     
    Show all issues

    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.

  12. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
    Show all issues

    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.

  13. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
     
    Show all issues

    Here's some places we do want a blank line. Any time you are:

    1. Separating two blocks from each other (exception being something like if/elif/else or try/except/finally)
    2. Separating statement from a block (like a variable followed by an if or for)
    3. Separating a block from a statement

    Think of each for loop as its own paragraph. It'll make it easier to read.

  14. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Since the line_info[1] values are never going to change, you'll be better served with elif blocks.

  15. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
    Show all issues

    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 do from __future__ import unicode_literals (which should be every single Python file we write).

  16. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
     
    Show all issues

    You'll want a blank line between these (separating a statement from a block).

  17. djblets/log/tests/test_log_view.py (Diff revision 6)
     
     
    Show all issues

    While commonly done in Python programs, it's actually dangerous for us to assign anything to _. That's because _ is used as an alias for ugettext, 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.

  18. djblets/log/views.py (Diff revision 6)
     
     
     
     
    Show all issues

    Looks like a blank line snuck in here.

  19. 
      
xiaole2
Review request changed
Commit:
3d3eeeaf89212f8ad505a0ef9887d870c0221c36
2e7340ef24aa1eda0f070dcb0941400729180b62

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

xiaole2
mike_conley
  1. Thanks, Xiaole! Nice tests. Just one thing from what I can tell - see below.

  2. djblets/log/tests/test_log_view.py (Diff revision 8)
     
     
    Show all issues

    unicode(name) doesn't work in Python 3 because the unicode 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.

    1. Do we need the unicode thing at all? - We don't need this thing. I have removed them and the tests all passed.
      self.assertEqual(name, 'By date') pass

  3. 
      
xiaole2
Review request changed
Change Summary:

removed unicode() and all tests pass

Commit:
e88ef3ff6171c8df91636705916c36a3d1cc06a0
2b03f4b44e507ce16e81cceeff03ae2e4d76049a

Checks run (2 succeeded)

flake8 passed.
JSHint passed.