diff --git a/pavelib/paver_tests/test_prereqs.py b/pavelib/paver_tests/test_prereqs.py index 01fa3d6167..5f416a884c 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 65aa48be8b..8f7fb69eaa 100644 --- a/pavelib/paver_tests/utils.py +++ b/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 diff --git a/pavelib/prereqs.py b/pavelib/prereqs.py index 09f66c0f1f..f44da6eaa5 100644 --- a/pavelib/prereqs.py +++ b/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 ))