Merge pull request #23751 from alanoe/alanoe/fix_npm_install_error_handling_in_master

Fix 'npm install' command execution error handling

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

@@ -79,15 +79,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

@@ -138,24 +138,20 @@ def node_prereqs_installation():
     npm_log_file = open(npm_log_file_path, 'wb')
     npm_command = 'npm install --verbose'.split()
 
-    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 str(error):
-            print("npm install error detected. Retrying...")
-            proc = subprocess.Popen(npm_command, stderr=npm_log_file)
-            proc.wait()
-        else:
-            raise
+        retcode = proc.wait()
+        if retcode == 1:
+            raise Exception("npm install failed: See {}".format(npm_log_file_path))
     print("Successfully installed NPM packages. Log found at {}".format(
         npm_log_file_path
     ))