Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Django 2.0 compatibility and improved test coverage #60

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,4 @@ cover/
.cache/
htmlcov/
coverage.xml
/.pytest_cache
1 change: 0 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ language: python
sudo: false
python:
- '2.7'
- '3.4'
- '3.5'
- '3.6'
install: travis_retry pip install -U codecov tox-travis
Expand Down
25 changes: 13 additions & 12 deletions django_celery_monitor/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,12 @@
from django.shortcuts import render_to_response
from django.template import RequestContext
from django.utils.encoding import force_text
from django.utils.html import escape
from django.utils.html import escape, format_html
from django.utils.translation import ugettext_lazy as _

from celery import current_app
from celery import states
from celery.task.control import broadcast, revoke, rate_limit
from celery.utils.text import abbrtask

from .models import TaskState, WorkerState
from .humanize import naturaldate
Expand Down Expand Up @@ -48,7 +47,8 @@ def colored_state(task):
"""
state = escape(task.state)
color = TASK_STATE_COLORS.get(task.state, 'black')
return '<b><span style="color: {0};">{1}</span></b>'.format(color, state)
return format_html('<b><span style="color: {0};">{1}</span></b>', color,
state)


@display_field(_('state'), 'last_heartbeat')
Expand All @@ -59,14 +59,15 @@ def node_state(node):
"""
state = node.is_alive() and 'ONLINE' or 'OFFLINE'
color = NODE_STATE_COLORS[state]
return '<b><span style="color: {0};">{1}</span></b>'.format(color, state)
return format_html('<b><span style="color: {0};">{1}</span></b>', color,
state)


@display_field(_('ETA'), 'eta')
def eta(task):
"""Return the task ETA as a grey "none" if none is provided."""
if not task.eta:
return '<span style="color: gray;">none</span>'
return format_html('<span style="color: gray;">none</span>')
return escape(make_aware(task.eta))


Expand All @@ -78,18 +79,18 @@ def tstamp(task):
it as a "natural date" -- a human readable version.
"""
value = make_aware(task.tstamp)
return '<div title="{0}">{1}</div>'.format(
escape(str(value)), escape(naturaldate(value)),
)
return format_html('<div title="{0}">{1}</div>', str(value),
naturaldate(value))


@display_field(_('name'), 'name')
def name(task):
"""Return the task name and abbreviates it to maximum of 16 characters."""
short_name = abbrtask(task.name, 16)
return '<div title="{0}"><b>{1}</b></div>'.format(
escape(task.name), escape(short_name),
)
short_name = task.name
if task.name and len(task.name) > 16:
short_name = format_html('{0}&hellip;', task.name[0:15])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please continue to use abbrtask here since it does more than what this code does. Just cutting off the task name would make identifiying a task with a long import path hard to read.

E.g. Imagine a task with the name myproject.some_complex_app.under_module.tasks.version_2.whatever_task (something that I've seen myself). In that case I wouldn't even see the task name with this new code. Instead abbrtask will show [.]whatever_task.

Copy link
Author

@pieterdd pieterdd Mar 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I'm sympathetic to your concerns about the way it abbreviates names, sticking with abbrtask from Celery simply didn't seem like an option.

The abbrtask function from Celery doesn't actually do what the docstring of the name function promises. It doesn't cut off at the requested amount of characters. So when I wrote unit tests for the name function, I found behavior that contradicts promises made in the docstring.

I had to choose between:

  • Rewriting the failing test to accept incorrect behavior
  • Sticking with the abbrtask function but doing postprocessing on it as a workaround
  • Stop using the abbrtask function

I went for option 3. Would you make a different choice?

return format_html('<div title="{0}"><b>{1}</b></div>', task.name,
short_name)


class ModelMonitor(admin.ModelAdmin):
Expand Down
20 changes: 12 additions & 8 deletions django_celery_monitor/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
from django.conf import settings
from django.db.models import DateTimeField, Func
from django.utils import timezone
from django.utils.html import escape
from django.utils.html import format_html
from six import string_types

try:
from django.db.models.functions import Now
Expand Down Expand Up @@ -92,21 +93,24 @@ def action(short_description, **kwargs):


def fixedwidth(field, name=None, pt=6, width=16, maxlen=64, pretty=False):
"""Render a field with a fixed width."""
"""Render a field with a fixed width.

:param bool pretty:
For field values that are not a string, use pretty printing if True.
"""
@display_field(name or field, field)
def f(task):
val = getattr(task, field)
if pretty:

# Pretty printing only makes sense for things that aren't strings
if not isinstance(val, string_types) and pretty:
val = pformat(val, width=width)
if val.startswith("u'") or val.startswith('u"'):
val = val[2:-1]
shortval = val.replace(',', ',\n')
shortval = shortval.replace('\n', '|br/|')
shortval = shortval.replace('\n', '<br/>')

if len(shortval) > maxlen:
shortval = shortval[:maxlen] + '...'
styled = FIXEDWIDTH_STYLE.format(
escape(val[:255]), pt, escape(shortval),
)
return styled.replace('|br/|', '<br/>')
return format_html(FIXEDWIDTH_STYLE, val[:255], pt, shortval)
return f
6 changes: 6 additions & 0 deletions requirements/test.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,9 @@ case>=1.3.1
pytest>=3.0
pytest-django==3.1.2
pytz>dev

# Allows freezing of current time in tests
freezegun==0.3.9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is 0.3.10, please update.


# Provides unittest.mock when running the test suite on Python 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That comment seems wrong, the mock package doesn't provide unittest.mock but mock. unittest.mock just happens to be a port to Python 3.x which we could use if we'd only support Python 3.x.

mock==2.0.0
2 changes: 2 additions & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ def add_default(m):
def add_doc(m):
return (('doc', m.groups()[0]),)


pats = {re_meta: add_default,
re_doc: add_doc}
here = os.path.abspath(os.path.dirname(__file__))
Expand Down Expand Up @@ -111,6 +112,7 @@ def _reqs(*f):
def reqs(*f):
return [req for subreq in _reqs(*f) for req in subreq]


# -*- Long Description -*-

if os.path.exists('README.rst'):
Expand Down
76 changes: 76 additions & 0 deletions tests/unit/test_admin.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
from __future__ import absolute_import, unicode_literals

from datetime import datetime
from unittest import TestCase
from mock import Mock

from freezegun import freeze_time

from django_celery_monitor.admin import colored_state, node_state, eta,\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use parenthesis for implied line-continuation instead of a backslash here.

tstamp, name


class ColoredStateTest(TestCase):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move all the tests under a common TestCase class, e.g. AdminDisplayFieldTests and use proper unittest assert methods (self.assertEqual etc) instead of the assert statement.

def test_success(self):
"""Rendering of a sucessful task's state."""
mock_task = Mock()
mock_task.state = 'SUCCESS'
expected_html = '<b><span style="color: green;">SUCCESS</span></b>'
assert colored_state(mock_task) == expected_html


class NodeStateTest(TestCase):
def test_online(self):
"""Rendering of an online node's state."""
mock_node = Mock()
mock_node.is_alive.return_value = True
expected_html = '<b><span style="color: green;">ONLINE</span></b>'
assert node_state(mock_node) == expected_html


class ETATest(TestCase):
def test_eta_value_not_set(self):
"""Rendering when no ETA is specified by the task."""
mock_task = Mock()
mock_task.eta = None
expected_html = '<span style="color: gray;">none</span>'
assert eta(mock_task) == expected_html

def test_eta_value_set(self):
"""Rendering when an ETA value is set by the task."""
mock_task = Mock()
mock_task.eta = datetime(2020, 1, 2, 8, 35)
expected_html = '2020-01-02 08:35:00+00:00'
assert eta(mock_task) == expected_html


class TStampTest(TestCase):
def test_time_difference_of_roughly_two_years(self):
"""Timestamp is roughly two years into the past."""
mock_task = Mock()
mock_task.tstamp = datetime(2015, 8, 5, 17, 3)
expected_html = '<div title="2015-08-05 17:03:00+00:00">2 years ago' \
'</div>'
with freeze_time(datetime(2017, 8, 6)):
assert tstamp(mock_task) == expected_html


class NameTest(TestCase):
def test_no_more_than_16_characters(self):
"""
No truncation should happen on names that are at most 16 characters in
length.
"""
mock_task = Mock()
mock_task.name = '1234567890123456'
expected_html = '<div title="1234567890123456"><b>1234567890123456' \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use backslashes for line continuation.

'</b></div>'
assert name(mock_task) == expected_html

def test_more_than_16_characters(self):
"""Truncation should happen above 16 characters."""
mock_task = Mock()
mock_task.name = '12345678901234567'
expected_html = '<div title="12345678901234567"><b>123456789012345' \
'&hellip;</b></div>'
assert name(mock_task) == expected_html
40 changes: 40 additions & 0 deletions tests/unit/test_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
from __future__ import absolute_import, unicode_literals

from re import match
from unittest import TestCase

from mock import Mock

from django_celery_monitor.utils import fixedwidth


class FixedWidthTest(TestCase):
def test_unpretty_string(self):
"""Do not request pretty printing for a string."""
task_mock = Mock()
task_mock.name = 'testing'
expected_html = '<span title="testing" style="font-size: 6pt; font-' \
'family: Menlo, Courier; ">testing</span> '
assert fixedwidth('name')(task_mock) == expected_html

def test_pretty_string(self):
"""Request pretty printed value for a string."""
task_mock = Mock()
task_mock.name = 'testing'
expected_html = ('<span title="testing" style="font-size:'
' 6pt; font-family: Menlo, Courier; ">testing'
'</span> ')
assert fixedwidth('name', pretty=True)(task_mock) == expected_html

def test_pretty_object(self):
"""Request pretty printed value for an object."""
task_mock = Mock()
task_mock.name = object()
actual_html = fixedwidth('name', pretty=True)(task_mock)
matcher = match(
'<span title="&lt;object object at 0x[0-9a-f]+&gt;" style="'
'font-size: 6pt; font-family: Menlo, Courier; ">'
'&lt;object object at 0x[0-9a-f]+&gt;</span> ',
actual_html,
)
assert matcher
8 changes: 3 additions & 5 deletions tox.ini
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[tox]
envlist =
tests-py{py,27,34,35}-dj{18,19,110}
tests-py36-dj111
tests-py{27,35,36}-dj111
tests-py{35,36}-dj20
apicheck
builddocs
flake8
Expand All @@ -24,10 +24,8 @@ deps=
-r{toxinidir}/requirements/test.txt
-r{toxinidir}/requirements/test-ci.txt

dj18: django>=1.8,<1.9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't remove 1.8, 1.9. and 1.10 yet.

dj19: django>=1.9,<1.10
dj110: django>=1.10,<1.11
dj111: django>=1.11,<2
dj20: django>=2.0,<2.1

apicheck,builddocs,linkcheck: -r{toxinidir}/requirements/docs.txt
flake8,flakeplus,manifest,pydocstyle,readme: -r{toxinidir}/requirements/pkgutils.txt
Expand Down