From 30495b45808a278579759ebaed0a645056098534 Mon Sep 17 00:00:00 2001
From: Alan Evangelista <alan@opencraft.com>
Date: Sun, 15 Mar 2020 08:50:35 -0300
Subject: [PATCH] Fix 'npm install' command execution error handling

'npm install' command execution was changed from paver.easy.sh()
to subprocess.Popen() in commit 0dd13fad1bced76d77423e7f4d01b93c54930757,
but exception handling was not updated accordingly. Due to this,
errors in 'npm install' execution were not detected.
---
 pavelib/paver_tests/test_prereqs.py | 13 ++++++++-----
 pavelib/paver_tests/utils.py        | 10 +++-------
 pavelib/prereqs.py                  | 30 ++++++++++++++---------------
 3 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/pavelib/paver_tests/test_prereqs.py b/pavelib/paver_tests/test_prereqs.py
index f315a04da3..669ab6bed8 100644
--- a/pavelib/paver_tests/test_prereqs.py
+++ b/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)
 
diff --git a/pavelib/paver_tests/utils.py b/pavelib/paver_tests/utils.py
index f8e1c0f245..3262917d5c 100644
--- a/pavelib/paver_tests/utils.py
+++ b/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
diff --git a/pavelib/prereqs.py b/pavelib/prereqs.py
index 8b5cb5fb6f..9a1b4ad9cc 100644
--- a/pavelib/prereqs.py
+++ b/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
     ))