hook: switch to using check_call

beetsplug/hook.py

@@ -105,7 +105,10 @@ def hook_function(**kwargs):
                             u' '.join(command_pieces), event)
 
             try:
-                subprocess.Popen(command_pieces).wait()
+                subprocess.check_call(command_pieces)
+            except subprocess.CalledProcessError as exc:
+                self._log.error(u'hook for {0} exited with status {1}',
+                                event, exc.returncode)
             except OSError as exc:
                 self._log.error(u'hook for {0} failed: {1}', event, exc)
 

test/test_hook.py

@@ -20,7 +20,7 @@
 import unittest
 
 from test import _common
-from test.helper import TestHelper
+from test.helper import TestHelper, capture_log
 
 from beets import config
 from beets import plugins
@@ -37,7 +37,7 @@ class HookTest(_common.TestCase, TestHelper):
     TEST_HOOK_COUNT = 5
 
     def setUp(self):
-        self.setup_beets()  # Converter is threaded
+        self.setup_beets()
 
     def tearDown(self):
         self.unload_plugins()
@@ -54,6 +54,38 @@ def _add_hook(self, event, command):
 
         config['hook']['hooks'] = hooks
 
+    def test_hook_empty_command(self):
+        self._add_hook('test_event', '')
+
+        self.load_plugins('hook')
+
+        with capture_log('beets.hook') as logs:
+            plugins.send('test_event')
+
+        self.assertIn('hook: invalid command ""', logs)
+
+    def test_hook_non_zero_exit(self):
+        self._add_hook('test_event', 'sh -c "exit 1"')
+
+        self.load_plugins('hook')
+
+        with capture_log('beets.hook') as logs:
+            plugins.send('test_event')
+
+        self.assertIn('hook: hook for test_event exited with status 1', logs)
+
+    def test_hook_non_existent_command(self):
+        self._add_hook('test_event', 'non-existent-command')
+
+        self.load_plugins('hook')
+
+        with capture_log('beets.hook') as logs:
+            plugins.send('test_event')
+
+        self.assertTrue(any(
+            message.startswith("hook: hook for test_event failed: ")
+            for message in logs))
+
     def test_hook_no_arguments(self):
         temporary_paths = [
             get_temporary_path() for i in range(self.TEST_HOOK_COUNT)