Merge lp:~mars/tarmac/fix-test-command-call into lp:~launchpad/tarmac/lp-tarmac

[Māris Fogels]

Fixed calling the program defined by bundle_verify_command.

Hi,

This branch fixes calls to the program defined by bundle_verify_command. The subprocess call for the command must use a file that is opened in 'w' mode, not 'rw'.

I also took a moment to clean up how the current working directory for the subprocess is set. I added comments and whitespace to make the function body more readable.

Maris

[Gary Poster (gary)]

Looks good, thank you.

As the smallest of niggles, line 37 of the diff feels like a bit too much whitespace within the function to me, but that's one of those "sprinkle to taste" things. Just a thought.

Thanks again

Gary

review: Approve

=== modified file 'tarmac/plugins/bundlecommand.py'
--- tarmac/plugins/bundlecommand.py	2011-01-05 17:09:54 +0000
+++ tarmac/plugins/bundlecommand.py	2011-01-10 22:15:30 +0000
@@ -136,6 +136,7 @@
         proposals merged into target and then runs the verify command.
         Updating the list of proposals in case of failure.
         """
+        # Try and find the command that runs the test suite.
         try:
             self.verify_command = target.config.bundle_verify_command
         except AttributeError:
@@ -149,11 +150,7 @@
             except AttributeError:
                 return
 
-        start_time = datetime.datetime.utcnow()
-        self.logger.debug('Running test command: %s' % self.verify_command)
-        cwd = os.getcwd()
-        os.chdir(target.tree.abspath(''))
-        # Output must be logged and kept in the filesystem to help debugging.
+        # Rotate the test output logs.
         current_log = os.path.join(
             target.config.debug_log_location, 'current.log')
         previous_log = os.path.join(
@@ -161,39 +158,43 @@
         if os.path.exists(current_log):
             # Move it out of the way. Trashes previous.log.
             os.rename(current_log, previous_log)
-        # Create a new empty file to log the output.
-        open(current_log, 'w').close()
-        # File must be opened for read and write.
-        output = open(current_log, 'rw')
+
+        # Run our test command.
+        start_time = datetime.datetime.utcnow()
+        self.logger.debug('Running test command: %s' % self.verify_command)
+
+        output = open(current_log, 'w')
         try:
             proc = subprocess.Popen(
                 self.verify_command,
                 shell=True,
+                cwd=target.tree.abspath(''),
                 stdout=output,
                 stderr=subprocess.STDOUT)
             return_code = proc.wait()
-            output.seek(0)
-            os.chdir(cwd)
-            self.logger.debug('Completed test command: %s' % self.verify_command)
-            end_time = datetime.datetime.utcnow()
-
-            if return_code != 0:
-                for source, proposal in collected_proposals:
-                    self.do_failed(
-                        target, start_time, end_time, proposal, output)
-                # XXX matsubara 2010-12-17: The command code should be
-                # handling this, but due to the way we want emails formatted
-                # and sent, this is done here for now.
-                # See bug 691563 for more details.
-                # Uncommit and revert changes up to the point where target
-                # was before the merge.
-                target.uncommit_and_revert(restore_revno)
-            else:
-                self.logger.info('==========')
-                self.logger.info(output.read())
         finally:
             output.close()
 
+        self.logger.debug('Completed test command: %s' % self.verify_command)
+        end_time = datetime.datetime.utcnow()
+
+        # Handle the test command output.
+        testlog = open(current_log)
+        if return_code != 0:
+            for source, proposal in collected_proposals:
+                self.do_failed(
+                    target, start_time, end_time, proposal, testlog)
+            # XXX matsubara 2010-12-17: The command code should be
+            # handling this, but due to the way we want emails formatted
+            # and sent, this is done here for now.
+            # See bug 691563 for more details.
+            # Uncommit and revert changes up to the point where target
+            # was before the merge.
+            target.uncommit_and_revert(restore_revno)
+        else:
+            self.logger.info('==========')
+            self.logger.info(testlog.read())
+
     def do_failed(self, target, start_time, end_time, proposal, output):
         '''Perform failure tests.