Code review comment for lp:~ack/landscape-client/package-reporter-permissions

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Nice branch, +1!

[1]

+def spawn_process(executable, args=(), env={}, path=None, uid=None, gid=None,
+ wait_pipes=True):

Please add a @param for describing what wait_pipes does.

[2]

+ def tearDown(self):
+ os.unlink(self.command)

No need for this, everything created with self.makeFile will be automatically removed.

[3]
        """
        The process returns the expected standard error.
        """

        self._write_command("#!/bin/sh\necho -n $@ >&2")

There's an extra blank between the docstring and the first line of the test.

[4]

+ def _write_command(self, command):

I'd suggest to drop this and use landscape.lib.fs.create_file instead (which maybe should be renamed write_file):

from landscape.lib.fs import create_file

    def test_spawn_process_return_value(self):
        create_file(self.command, "#!/bin/sh\nexit 2")

        def callback((out, err, code)):

This is because the less ad-hoc methods the test has the more readable it is.

review: Approve

« Back to merge proposal