|
|
|
Created:
12 years, 11 months ago by j.c.sackett Modified:
12 years, 11 months ago Visibility:
Public. |
Adds queue handling tests for jobs
This adds a number of tests to handle queue management for the jobs' pipeline.
Where possible, it also adds tests for expected outputs and processes within
the jobs. This is not comprehensive--the jobs need more tests, esp where mongo
and other data sources are being worked with (e.g. indexing).
There is also some drive by cleanup and linting, particularly removing unused
code.
https://code.launchpad.net/~jcsackett/charmworld/better-job-tests/+merge/145870
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 10
Patch Set 2 : Adds queue handling tests for jobs #
Total comments: 1
Total messages: 5
|
j.c.sackett
Please take a look.
|
12 years, 11 months ago (2013年01月31日 14:36:11 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
drive by whilst on holiday (ie. +0) just some comments. https://codereview.appspot.com/7223066/diff/1/charmworld/jobs/scan.py File charmworld/jobs/scan.py (right): https://codereview.appspot.com/7223066/diff/1/charmworld/jobs/scan.py#newcode136 charmworld/jobs/scan.py:136: # probably a candidate for deletion. its used for reindexing what's on disk by substituting in the main func https://codereview.appspot.com/7223066/diff/1/charmworld/jobs/tests/test_bzr.py File charmworld/jobs/tests/test_bzr.py (right): https://codereview.appspot.com/7223066/diff/1/charmworld/jobs/tests/test_bzr.... charmworld/jobs/tests/test_bzr.py:17: def getServer(self, in_queue): naming isn't per the convention of the rest of the code base. https://codereview.appspot.com/7223066/diff/1/charmworld/jobs/tests/test_bzr.... charmworld/jobs/tests/test_bzr.py:18: server = redis.Redis() bad indentation from cut-n-paste https://codereview.appspot.com/7223066/diff/1/charmworld/jobs/tests/test_bzr.... charmworld/jobs/tests/test_bzr.py:25: logger = logging.getLogger("charm.bzr") capturing a log stream should be a test helper method in a charmworld.testing.BaseClass https://codereview.appspot.com/7223066/diff/1/charmworld/jobs/tests/test_chan... File charmworld/jobs/tests/test_changelog.py (right): https://codereview.appspot.com/7223066/diff/1/charmworld/jobs/tests/test_chan... charmworld/jobs/tests/test_changelog.py:27: def getServer(self, in_queue): please make a job testing base class with both of these methods (named per convention)
I'm with kapil on the need to create a job test base class with helpers for the log capturing and redis queue setup. https://codereview.appspot.com/7223066/diff/1/charmworld/jobs/tests/test_bzr.py File charmworld/jobs/tests/test_bzr.py (right): https://codereview.appspot.com/7223066/diff/1/charmworld/jobs/tests/test_bzr.... charmworld/jobs/tests/test_bzr.py:7: import redis Garden imports, alphabetical together. https://codereview.appspot.com/7223066/diff/1/charmworld/jobs/tests/test_chan... File charmworld/jobs/tests/test_changelog.py (right): https://codereview.appspot.com/7223066/diff/1/charmworld/jobs/tests/test_chan... charmworld/jobs/tests/test_changelog.py:8: import redis import garden https://codereview.appspot.com/7223066/diff/1/charmworld/jobs/tests/test_lp.py File charmworld/jobs/tests/test_lp.py (right): https://codereview.appspot.com/7223066/diff/1/charmworld/jobs/tests/test_lp.p... charmworld/jobs/tests/test_lp.py:6: import redis Garden imports https://codereview.appspot.com/7223066/diff/1/charmworld/testing/__init__.py File charmworld/testing/__init__.py (right): https://codereview.appspot.com/7223066/diff/1/charmworld/testing/__init__.py#... charmworld/testing/__init__.py:102: def test_redis_queues(): can these be patch_redis_queues and be more descriptive and not require the @nottest? https://codereview.appspot.com/7223066/diff/1/charmworld/testing/__init__.py#... charmworld/testing/__init__.py:133: def get_bad_mock(exception_type=Exception, *args): can we add a docstring on what this is for? get_bad_mock seems a bit generic when viewed from this file.
Please take a look.
lgtm thanks for the changes. Just a note to add a bug for the new XXX so it's tracked. https://codereview.appspot.com/7223066/diff/10001/charmworld/jobs/scan.py File charmworld/jobs/scan.py (right): https://codereview.appspot.com/7223066/diff/10001/charmworld/jobs/scan.py#new... charmworld/jobs/scan.py:164: # XXX j.c.sackett scan_repo is swappedx for scan charm to reindex Please add a bug for this and maybe it could be a drive by in the queue worker card.