Fix 'npm install' command execution error handling

'npm install' command execution was changed from paver.easy.sh()
to subprocess.Popen() in commit 0dd13fad1b,
but exception handling was not updated accordingly. Due to this,
errors in 'npm install' execution were not detected.

pavelib/paver_tests/test_prereqs.py

@@ -6,6 +6,7 @@ Tests covering the Open edX Paver prequisites installation workflow
 import os
 import unittest
 
+import mock
 from mock import patch
 from paver.easy import BuildFailure
 
@@ -89,14 +90,16 @@ class TestPaverNodeInstall(PaverTestCase):
 
     def test_npm_install_with_subprocess_error(self):
         """
-        An exit with subprocess exit 1 is what paver receives when there is
-        an npm install error ("cb() never called!"). Test that we can handle
-        this kind of failure. For more info see TE-1767.
+        Test an error in 'npm install' execution
         """
         with patch('subprocess.Popen') as _mock_popen:
-            _mock_popen.side_effect = fail_on_npm_install
-            with self.assertRaises(BuildFailure):
+            _mock_subprocess = mock.Mock()
+            attrs = {'wait': fail_on_npm_install}
+            _mock_subprocess.configure_mock(**attrs)
+            _mock_popen.return_value = _mock_subprocess
+            with self.assertRaises(Exception):
                 pavelib.prereqs.node_prereqs_installation()
+
         # npm install will be called twice
         self.assertEqual(_mock_popen.call_count, 2)
 

pavelib/paver_tests/utils.py

@@ -80,15 +80,11 @@ def fail_on_eslint(*args, **kwargs):
             return
 
 
-def fail_on_npm_install(*args, **kwargs):  # pylint: disable=unused-argument
+def fail_on_npm_install():
     """
-    For our tests, we need the call for diff-quality running pycodestyle reports to fail, since that is what
-    is going to fail when we pass in a percentage ("p") requirement.
+    Used to simulate an error when executing "npm install"
     """
-    if ["npm", "install", "--verbose"] == args[0]:
-        raise BuildFailure('Subprocess return code: 1')
-    else:
-        return
+    return 1
 
 
 def unexpected_fail_on_npm_install(*args, **kwargs):  # pylint: disable=unused-argument

pavelib/prereqs.py

@@ -143,23 +143,21 @@ def node_prereqs_installation():
 
     cb_error_text = "Subprocess return code: 1"
 
-    # Error handling around a race condition that produces "cb() never called" error. This
-    # evinces itself as `cb_error_text` and it ought to disappear when we upgrade
-    # npm to 3 or higher. TODO: clean this up when we do that.
-    try:
-        # The implementation of Paver's `sh` function returns before the forked
-        # actually returns. Using a Popen object so that we can ensure that
-        # the forked process has returned
+    # The implementation of Paver's `sh` function returns before the forked
+    # actually returns. Using a Popen object so that we can ensure that
+    # the forked process has returned
+    proc = subprocess.Popen(npm_command, stderr=npm_log_file)
+    retcode = proc.wait()
+    if retcode == 1:
+        # Error handling around a race condition that produces "cb() never called" error. This
+        # evinces itself as `cb_error_text` and it ought to disappear when we upgrade
+        # npm to 3 or higher. TODO: clean this up when we do that.
+        print("npm install error detected. Retrying...")
         proc = subprocess.Popen(npm_command, stderr=npm_log_file)
-        proc.wait()
-    except BuildFailure as error:
-        if cb_error_text in six.text_type(error):
-            print("npm install error detected. Retrying...")
-            proc = subprocess.Popen(npm_command, stderr=npm_log_file)
-            proc.wait()
-        else:
-            raise
-    print(u"Successfully installed NPM packages. Log found at {}".format(
+        retcode = proc.wait()
+        if retcode == 1:
+            raise Exception("npm install failed")
+    print("Successfully installed NPM packages. Log found at {}".format(
         npm_log_file_path
     ))