In LogFile.destroy(), you set self.valid=False no matter what exception
os.remove() raises. Is this intended?
_get_lines() has the assert-parens, but more importantly, the implementation
can probably be rewritten like so:
with open(self.path) as fh:
return fh.readlines()
IOW, .readlines() already returns a list, so you don't need to create the empty
list before the loop. Another thing to think about here is what encoding you
want self.path to be read using. Since you're opening it in text mode, it will
be in a platform specific (i.e. not predictable ;) encoding. Maybe set
encoding='utf-8'?
In readlines(), see my previous comment about time.sleep(), timeout-based
loops, and the race conditions in using a .exists() test rather than catching
FileNotFound errors.
SystemInit.destroy() isn't needed if all it does is upcall to super().destroy().
Is there a specific reason that InotifyHandler is defined inside SessionInit
rather than at module global?
Given that you want strings from _get_sessions(), you can probably do this
instead:
sessions = {}
for line in subprocess.check_output(args,
universal_newlines=True).splitlines():
pid, socket = line.split()
sessions[pid] = socket
return sessions
Unnecessary backslashes in SessionInit.__init__()
I'd probably write the multiple args.appends as: args.extend([UPSTART,
'--user', '--confdir', self.conf_dir, '--logdir', self.log_dir)
with the appropriate line wrapping of course.
When checking for whether a key is in a dictionary, it's more efficient to write
if not str(self.pid) in sessions:
than to use sessions.keys().
In stop_session_init(), I'm not sure you need to use the with statement version
of assertRaises. This is useful for when you want to capture the exception and
make further assertions on it, but you only care that the exception gets
raised. This is probably good enough:
self.assertRaises(ProcessLookupError, os.kill, pid, 0)
(There's another example of this later on in test_session_init_reexec()).
In test_init_start_file_bridge(), you probably don't need to initialize lines =
[]. Just inside the open(), do
lines = f.readlines()
There are several examples of this in the file.
(also, you might want to open the file with an explicit encoding)
self.assertTrue(len(pids.keys()) == 1)
is probably better
self.assertEqual(len(pids), 1)
and
self.assertTrue(isinstance(pid, int))
can be written in Python 3.3 as
self.assertIsInstance(pid, int)
I think you leak the tempdir that you create with tempfile.mkdtemp(). At least
I can't see where you might shutil.rmtree() that directory.
In the test that starts with "got = False"; did you know that for loops can
have an 'else' clause?! It only gets triggered if the loop exits normally,
i.e. not when a break occurs. Given the way you're doing this test, it might
not make much of a difference, but you could write the loop like this:
for i in range(timeout):
output = subprocess.getoutput(cmd)
result = re.search('--state-fd\s+(\d+)', output)
if result:
break
time.sleep(1) # see previous comments about this style :)
else:
# We never hit the break
raise AssertionError('We never got the expected output')
Later...
self.assertTrue(len(pids.keys()) == 1)
is better:
self.assertEqual(len(pid), 1)
You don't necessarily need to call self.assertDictEqual() explicitly, since
self.assertEqual() will use this by default when comparing two dictionaries.
This is unnecessary, unless you plan on filling it out later:
def tearDown(self):
pass
Anyway, I hope these have been helpful! I'll happily review updates if you
want.
--
https://code.launchpad.net/~jamesodhunt/upstart/python-upstart-module/+merge/157549
Your team Upstart Reviewers is requested to review the proposed merge of
lp:~jamesodhunt/upstart/python-upstart-module into lp:upstart.
--
upstart-devel mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/upstart-devel
