[Merge] lp:~jamesodhunt/upstart/python-upstart-module into lp:upstart

[Barry Warsaw]

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.