Django 2.0 compatibility and improved test coverage #60

pieterdd · 2018-02-14T10:59:28Z

This pull request builds on #40. However, it was failing some code checks because - I assume - the test coverage was lacking. So here are their changes, some additional CI changes and extra unit tests.

This speeds up builds, and it's good practice to upgrade to Django 1.11 (last Python 2 version, LTS) or Django 2.0 anyway.

I discovered a bug in the name() function while writing these. The abbrtask() function from Celery doesn't actually do what the docstring of the name() function promises, so I changed the implementation to something simpler.

codecov · 2018-02-14T11:03:36Z

Codecov Report

Merging #60 into master will increase coverage by 7.64%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   72.58%   80.22%   +7.64%     
==========================================
  Files           9        9              
  Lines         434      435       +1     
  Branches       54       55       +1     
==========================================
+ Hits          315      349      +34     
+ Misses        114       73      -41     
- Partials        5       13       +8

Impacted Files	Coverage Δ
django_celery_monitor/admin.py	`64.75% <100%> (+11.03%)`	⬆️
django_celery_monitor/utils.py	`76.19% <100%> (+11.11%)`	⬆️
django_celery_monitor/humanize.py	`54.76% <0%> (+30.95%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ec1350...4bde111. Read the comment docs.

Also: - Remove usage of mark_safe that caused undesired side effects and theoretically allowed for cross-site scripting vulnerabilities. - Don't pretty print strings. This causes strange behavior that's inconsistent across certain Python/Django combinations.

pieterdd · 2018-02-14T15:00:49Z

All green! I've ensured that everything that @madisvain changed is now covered by tests.

DEKHTIARJonathan · 2018-02-28T12:43:58Z

@jezdez Do you think this PR will be ever merged ? I actively support the work of @pieterdd in this PR

jezdez · 2018-03-06T18:03:39Z

@DEKHTIARJonathan Yes, it will be merged once I've done the review, please don't ask questions like that.

jezdez

This needs some more work.

jezdez · 2018-03-06T18:08:47Z

requirements/test.txt

+# Allows freezing of current time in tests
+freezegun==0.3.9
+
+# Provides unittest.mock when running the test suite on Python 2


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.

jezdez · 2018-03-06T18:15:15Z

django_celery_monitor/admin.py

-    )
+    short_name = task.name
+    if task.name and len(task.name) > 16:
+        short_name = format_html('{0}&hellip;', task.name[0:15])


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.

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?

jezdez · 2018-03-06T18:15:43Z

requirements/test.txt

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


There is 0.3.10, please update.

jezdez · 2018-03-06T18:16:23Z

tests/unit/test_admin.py

+
+from freezegun import freeze_time
+
+from django_celery_monitor.admin import colored_state, node_state, eta,\


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

jezdez · 2018-03-06T18:20:13Z

tests/unit/test_admin.py

+    tstamp, name
+
+
+class ColoredStateTest(TestCase):


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.

jezdez · 2018-03-06T18:20:55Z

tests/unit/test_admin.py

+        """
+        mock_task = Mock()
+        mock_task.name = '1234567890123456'
+        expected_html = '<div title="1234567890123456"><b>1234567890123456' \


Please don't use backslashes for line continuation.

jezdez · 2018-03-06T18:22:25Z

tox.ini

@@ -24,10 +24,8 @@ deps=
    -r{toxinidir}/requirements/test.txt
    -r{toxinidir}/requirements/test-ci.txt

-    dj18: django>=1.8,<1.9


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

DEKHTIARJonathan · 2018-04-05T13:59:21Z

@pieterdd : I have the event worker which crashed without any reason without leaving anything on the logs every +/- 24hours. Do you think this could be caused by your modifications ?

I have opened an issue on the official celery repository about this: celery/celery#4623

pieterdd · 2018-04-05T16:05:14Z

You mention using v4.2 in the ticket. As far as I know, this is pre-release software.

tony · 2019-06-19T16:30:15Z

@pieterdd Can you try rebasing?

madisvain and others added 14 commits December 8, 2017 00:03

format html

037d262

admin updates

9df9301

fixed columns format_html

49ab46c

add one more format_html

a093f6f

finalize fix

7d77c13

Merge remote-tracking branch 'remote/master'

5f5e3a6

remove gitignore statements

9960816

fix linelength

6d946fc

remove unused import

20e9b99

fix all flake issues

9835972

Target Django 2.0.x

ee0eeec

Run Django 1.11 tests against Python 2 and 3

577fdc8

Drop support for old insecure Django versions

8c8a61c

This speeds up builds, and it's good practice to upgrade to Django 1.11 (last Python 2 version, LTS) or Django 2.0 anyway.

Write tests for the admin helpers

4db898c

I discovered a bug in the name() function while writing these. The abbrtask() function from Celery doesn't actually do what the docstring of the name() function promises, so I changed the implementation to something simpler.

pieterdd force-pushed the improve-tests branch 3 times, most recently from cfa9521 to 0619733 Compare February 14, 2018 14:36

Pieter De Decker added 2 commits February 14, 2018 15:49

Add tests for fixedwidth

a85ee42

Also: - Remove usage of mark_safe that caused undesired side effects and theoretically allowed for cross-site scripting vulnerabilities. - Don't pretty print strings. This causes strange behavior that's inconsistent across certain Python/Django combinations.

Add /.pytest_cache to .gitignore

4bde111

pieterdd force-pushed the improve-tests branch from 0619733 to 4bde111 Compare February 14, 2018 14:49

pieterdd mentioned this pull request Feb 14, 2018

Allow tags fix for Django 2.0 #40

Open

jezdez requested changes Mar 6, 2018

View reviewed changes

Jibinjohnkj mentioned this pull request Jul 24, 2019

Allow tags support has been removed in Django 2.0 #39

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Django 2.0 compatibility and improved test coverage #60

Django 2.0 compatibility and improved test coverage #60

pieterdd commented Feb 14, 2018

codecov bot commented Feb 14, 2018 •

edited

Loading

pieterdd commented Feb 14, 2018

DEKHTIARJonathan commented Feb 28, 2018

jezdez commented Mar 6, 2018

jezdez left a comment

jezdez Mar 6, 2018

jezdez Mar 6, 2018

pieterdd Mar 8, 2018 •

edited

Loading

jezdez Mar 6, 2018

jezdez Mar 6, 2018

jezdez Mar 6, 2018

jezdez Mar 6, 2018

jezdez Mar 6, 2018

DEKHTIARJonathan commented Apr 5, 2018

pieterdd commented Apr 5, 2018

tony commented Jun 19, 2019


		from freezegun import freeze_time

		from django_celery_monitor.admin import colored_state, node_state, eta,\

Django 2.0 compatibility and improved test coverage #60

Are you sure you want to change the base?

Django 2.0 compatibility and improved test coverage #60

Conversation

pieterdd commented Feb 14, 2018

codecov bot commented Feb 14, 2018 • edited Loading

Codecov Report

pieterdd commented Feb 14, 2018

DEKHTIARJonathan commented Feb 28, 2018

jezdez commented Mar 6, 2018

jezdez left a comment

Choose a reason for hiding this comment

jezdez Mar 6, 2018

Choose a reason for hiding this comment

jezdez Mar 6, 2018

Choose a reason for hiding this comment

pieterdd Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

jezdez Mar 6, 2018

Choose a reason for hiding this comment

jezdez Mar 6, 2018

Choose a reason for hiding this comment

jezdez Mar 6, 2018

Choose a reason for hiding this comment

jezdez Mar 6, 2018

Choose a reason for hiding this comment

jezdez Mar 6, 2018

Choose a reason for hiding this comment

DEKHTIARJonathan commented Apr 5, 2018

pieterdd commented Apr 5, 2018

tony commented Jun 19, 2019

codecov bot commented Feb 14, 2018 •

edited

Loading

pieterdd Mar 8, 2018 •

edited

Loading