From f52e3cf830dbf69eb5627c59583a8cb1705136c8 Mon Sep 17 00:00:00 2001 From: Ben Patterson Date: Thu, 10 Nov 2016 14:54:53 -0500 Subject: [PATCH 1/3] Workaround for `cb() never called` errors Problem: Intermittent "cb() never called!" errors on npm installs for platform. This is likely due to a race condition on installing OS-specific bits that are built during time of install. When more than one compilation takes place at one time, this error can occur. Workaround: Detect the error and re-run the install. (When retrying, one or all of the bits have been compiled. So running it again just finishes the install.) --- pavelib/prereqs.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/pavelib/prereqs.py b/pavelib/prereqs.py index 9612dd022e..fa6499361f 100644 --- a/pavelib/prereqs.py +++ b/pavelib/prereqs.py @@ -8,7 +8,7 @@ import os import re import sys -from paver.easy import sh, task +from paver.easy import sh, task, BuildFailure from .utils.envs import Env from .utils.timer import timed @@ -132,10 +132,19 @@ def node_prereqs_installation(): """ Configures npm and installs Node prerequisites """ + cb_error_text = "Subprocess return code: 1" sh("test `npm config get registry` = \"{reg}\" || " "(echo setting registry; npm config set registry" " {reg})".format(reg=NPM_REGISTRY)) - sh('npm install') + + # Error handling around a race condition that produces "cb() never called" error. This + # ought to disappear when we upgrade npm to 3 or higher. TODO: clean this up when we do that. + try: + sh('npm install') + except BuildFailure, error_text: + if cb_error_text in error_text: + print "npm install error detected. Retrying..." + sh('npm install') def python_prereqs_installation(): From 67be6807a462f2fc4a0ba77f034222a75199c5d9 Mon Sep 17 00:00:00 2001 From: Ben Patterson Date: Tue, 15 Nov 2016 17:46:07 -0500 Subject: [PATCH 2/3] Add tests. Enjoy your coverage and feel confident. --- pavelib/paver_tests/test_paver_quality.py | 41 +++--------------- pavelib/paver_tests/test_prereqs.py | 53 ++++++++++++++++++++++- pavelib/paver_tests/utils.py | 52 ++++++++++++++++++++++ pavelib/prereqs.py | 5 ++- 4 files changed, 115 insertions(+), 36 deletions(-) diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index 7c2810c621..2f36a26e0a 100644 --- a/pavelib/paver_tests/test_paver_quality.py +++ b/pavelib/paver_tests/test_paver_quality.py @@ -1,19 +1,21 @@ """ Tests for paver quality tasks """ -import os -from path import Path as path import tempfile import textwrap import unittest -from mock import patch, MagicMock, mock_open -from ddt import ddt, file_data -import pavelib.quality +import os import paver.easy import paver.tasks +from ddt import ddt, file_data +from mock import patch, MagicMock, mock_open +from path import Path as path from paver.easy import BuildFailure +import pavelib.quality +from pavelib.paver_tests.utils import CustomShMock + @ddt class TestPaverQualityViolations(unittest.TestCase): @@ -351,32 +353,3 @@ class TestPaverRunQuality(unittest.TestCase): self.assertEqual(_mock_pep8_violations.call_count, 1) # And assert that sh was called twice (for the call to "pylint" & "eslint") self.assertEqual(self._mock_paver_sh.call_count, 2) - - -class CustomShMock(object): - """ - Diff-quality makes a number of sh calls. None of those calls should be made during tests; however, some - of them need to have certain responses. - """ - - def fail_on_pylint(self, arg): - """ - For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what - is going to fail when we pass in a percentage ("p") requirement. - """ - if "pylint" in arg: - # Essentially mock diff-quality exiting with 1 - paver.easy.sh("exit 1") - else: - return - - def fail_on_eslint(self, arg): - """ - For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what - is going to fail when we pass in a percentage ("p") requirement. - """ - if "eslint" in arg: - # Essentially mock diff-quality exiting with 1 - paver.easy.sh("exit 1") - else: - return diff --git a/pavelib/paver_tests/test_prereqs.py b/pavelib/paver_tests/test_prereqs.py index 4c7ed9ba10..409a6fa163 100644 --- a/pavelib/paver_tests/test_prereqs.py +++ b/pavelib/paver_tests/test_prereqs.py @@ -4,7 +4,11 @@ Tests covering the Open edX Paver prequisites installation workflow import os import unittest -from pavelib.prereqs import no_prereq_install +from mock import call, patch +from paver.easy import BuildFailure +from pavelib.prereqs import no_prereq_install, node_prereqs_installation +from pavelib.paver_tests.utils import PaverTestCase, CustomShMock +from pavelib.paver_tests.test_paver_quality import CustomShMock class TestPaverPrereqInstall(unittest.TestCase): @@ -68,3 +72,50 @@ class TestPaverPrereqInstall(unittest.TestCase): Ensure that '1' will be True. """ self.check_val('1', True) + + +class TestPaverNodeInstall(PaverTestCase): + """ + Test node install logic + """ + + def setUp(self): + super(TestPaverNodeInstall, self).setUp() + os.environ['NO_PREREQ_INSTALL'] = 'false' + patcher = patch('pavelib.prereqs.sh', return_value=True) + self._mock_paver_sh = patcher.start() + self.addCleanup(patcher.stop) + + def test_npm_install_with_subprocess_error(self): + """ + Test that we handle a subprocess 1 (proxy for cb() never called error) + TE-1767 + """ + self._mock_paver_sh.side_effect = CustomShMock().fail_on_npm_install + with self.assertRaises(BuildFailure): + node_prereqs_installation() + actual_calls = self._mock_paver_sh.mock_calls + + # npm install will be called twice + self.assertEqual(actual_calls.count(call('npm install')), 2) + + def test_npm_install_called_once_when_successful(self): + """ + Vanilla npm install should only be calling npm install one time + """ + node_prereqs_installation() + actual_calls = self._mock_paver_sh.mock_calls + + # when there's no failure, npm install is only called once + self.assertEqual(actual_calls.count(call('npm install')), 1) + + def test_npm_install_with_unexpected_subprocess_error(self): + """ + If there's some other error, only call npm install once, and raise a failure + """ + self._mock_paver_sh.side_effect = CustomShMock().unexpected_fail_on_npm_install + with self.assertRaises(BuildFailure): + node_prereqs_installation() + actual_calls = self._mock_paver_sh.mock_calls + + self.assertEqual(actual_calls.count(call('npm install')), 1) diff --git a/pavelib/paver_tests/utils.py b/pavelib/paver_tests/utils.py index e4ca1b88f7..5cdfcd3fd3 100644 --- a/pavelib/paver_tests/utils.py +++ b/pavelib/paver_tests/utils.py @@ -1,9 +1,12 @@ """Unit tests for the Paver server tasks.""" import os +import paver.easy from paver import tasks from unittest import TestCase +from paver.easy import BuildFailure + class PaverTestCase(TestCase): """ @@ -58,3 +61,52 @@ class MockEnvironment(tasks.Environment): output = message if not output.startswith("--->"): self.messages.append(unicode(output)) + + +class CustomShMock(object): + """ + Diff-quality makes a number of sh calls. None of those calls should be made during tests; however, some + of them need to have certain responses. + """ + + def fail_on_pylint(self, arg): + """ + For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what + is going to fail when we pass in a percentage ("p") requirement. + """ + if "pylint" in arg: + # Essentially mock diff-quality exiting with 1 + paver.easy.sh("exit 1") + else: + return + + def fail_on_eslint(self, arg): + """ + For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what + is going to fail when we pass in a percentage ("p") requirement. + """ + if "eslint" in arg: + # Essentially mock diff-quality exiting with 1 + paver.easy.sh("exit 1") + else: + return + + def fail_on_npm_install(self, arg): + """ + For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what + is going to fail when we pass in a percentage ("p") requirement. + """ + if "npm install" in arg: + raise BuildFailure('Subprocess return code: 1') + else: + return + + def unexpected_fail_on_npm_install(self, arg): + """ + For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what + is going to fail when we pass in a percentage ("p") requirement. + """ + if "npm install" in arg: + raise BuildFailure('Subprocess return code: 50') + else: + return diff --git a/pavelib/prereqs.py b/pavelib/prereqs.py index fa6499361f..56974e0c1d 100644 --- a/pavelib/prereqs.py +++ b/pavelib/prereqs.py @@ -138,13 +138,16 @@ def node_prereqs_installation(): " {reg})".format(reg=NPM_REGISTRY)) # Error handling around a race condition that produces "cb() never called" error. This - # ought to disappear when we upgrade npm to 3 or higher. TODO: clean this up when we do that. + # 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: sh('npm install') except BuildFailure, error_text: if cb_error_text in error_text: print "npm install error detected. Retrying..." sh('npm install') + else: + raise BuildFailure(error_text) def python_prereqs_installation(): From 705c8b915e6d70b1fe2a0839e45645fb8adf3532 Mon Sep 17 00:00:00 2001 From: Ben Patterson Date: Thu, 17 Nov 2016 11:31:57 -0500 Subject: [PATCH 3/3] Udpates based on feedback. --- pavelib/paver_tests/test_paver_quality.py | 6 +- pavelib/paver_tests/test_prereqs.py | 17 +++-- pavelib/paver_tests/utils.py | 79 +++++++++++------------ 3 files changed, 52 insertions(+), 50 deletions(-) diff --git a/pavelib/paver_tests/test_paver_quality.py b/pavelib/paver_tests/test_paver_quality.py index 2f36a26e0a..2ae4bdfee3 100644 --- a/pavelib/paver_tests/test_paver_quality.py +++ b/pavelib/paver_tests/test_paver_quality.py @@ -14,7 +14,7 @@ from path import Path as path from paver.easy import BuildFailure import pavelib.quality -from pavelib.paver_tests.utils import CustomShMock +from pavelib.paver_tests.utils import fail_on_pylint, fail_on_eslint @ddt @@ -298,7 +298,7 @@ class TestPaverRunQuality(unittest.TestCase): """ # Underlying sh call must fail when it is running the pylint diff-quality task - self._mock_paver_sh.side_effect = CustomShMock().fail_on_pylint + self._mock_paver_sh.side_effect = fail_on_pylint _mock_pep8_violations = MagicMock(return_value=(0, [])) with patch('pavelib.quality._get_pep8_violations', _mock_pep8_violations): with self.assertRaises(SystemExit): @@ -318,7 +318,7 @@ class TestPaverRunQuality(unittest.TestCase): """ # Underlying sh call must fail when it is running the eslint diff-quality task - self._mock_paver_sh.side_effect = CustomShMock().fail_on_eslint + self._mock_paver_sh.side_effect = fail_on_eslint _mock_pep8_violations = MagicMock(return_value=(0, [])) with patch('pavelib.quality._get_pep8_violations', _mock_pep8_violations): with self.assertRaises(SystemExit): diff --git a/pavelib/paver_tests/test_prereqs.py b/pavelib/paver_tests/test_prereqs.py index 409a6fa163..dba0c748a6 100644 --- a/pavelib/paver_tests/test_prereqs.py +++ b/pavelib/paver_tests/test_prereqs.py @@ -7,8 +7,9 @@ import unittest from mock import call, patch from paver.easy import BuildFailure from pavelib.prereqs import no_prereq_install, node_prereqs_installation -from pavelib.paver_tests.utils import PaverTestCase, CustomShMock -from pavelib.paver_tests.test_paver_quality import CustomShMock +from pavelib.paver_tests.utils import ( + PaverTestCase, unexpected_fail_on_npm_install, fail_on_npm_install +) class TestPaverPrereqInstall(unittest.TestCase): @@ -81,17 +82,21 @@ class TestPaverNodeInstall(PaverTestCase): def setUp(self): super(TestPaverNodeInstall, self).setUp() + + # Ensure prereqs will be run os.environ['NO_PREREQ_INSTALL'] = 'false' + patcher = patch('pavelib.prereqs.sh', return_value=True) self._mock_paver_sh = patcher.start() self.addCleanup(patcher.stop) def test_npm_install_with_subprocess_error(self): """ - Test that we handle a subprocess 1 (proxy for cb() never called error) - TE-1767 + 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. """ - self._mock_paver_sh.side_effect = CustomShMock().fail_on_npm_install + self._mock_paver_sh.side_effect = fail_on_npm_install with self.assertRaises(BuildFailure): node_prereqs_installation() actual_calls = self._mock_paver_sh.mock_calls @@ -113,7 +118,7 @@ class TestPaverNodeInstall(PaverTestCase): """ If there's some other error, only call npm install once, and raise a failure """ - self._mock_paver_sh.side_effect = CustomShMock().unexpected_fail_on_npm_install + self._mock_paver_sh.side_effect = unexpected_fail_on_npm_install with self.assertRaises(BuildFailure): node_prereqs_installation() actual_calls = self._mock_paver_sh.mock_calls diff --git a/pavelib/paver_tests/utils.py b/pavelib/paver_tests/utils.py index 5cdfcd3fd3..168b9c4556 100644 --- a/pavelib/paver_tests/utils.py +++ b/pavelib/paver_tests/utils.py @@ -63,50 +63,47 @@ class MockEnvironment(tasks.Environment): self.messages.append(unicode(output)) -class CustomShMock(object): +def fail_on_eslint(arg): """ - Diff-quality makes a number of sh calls. None of those calls should be made during tests; however, some - of them need to have certain responses. + For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what + is going to fail when we pass in a percentage ("p") requirement. """ + if "eslint" in arg: + # Essentially mock diff-quality exiting with 1 + paver.easy.sh("exit 1") + else: + return - def fail_on_pylint(self, arg): - """ - For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what - is going to fail when we pass in a percentage ("p") requirement. - """ - if "pylint" in arg: - # Essentially mock diff-quality exiting with 1 - paver.easy.sh("exit 1") - else: - return - def fail_on_eslint(self, arg): - """ - For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what - is going to fail when we pass in a percentage ("p") requirement. - """ - if "eslint" in arg: - # Essentially mock diff-quality exiting with 1 - paver.easy.sh("exit 1") - else: - return +def fail_on_pylint(arg): + """ + For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what + is going to fail when we pass in a percentage ("p") requirement. + """ + if "pylint" in arg: + # Essentially mock diff-quality exiting with 1 + paver.easy.sh("exit 1") + else: + return - def fail_on_npm_install(self, arg): - """ - For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what - is going to fail when we pass in a percentage ("p") requirement. - """ - if "npm install" in arg: - raise BuildFailure('Subprocess return code: 1') - else: - return - def unexpected_fail_on_npm_install(self, arg): - """ - For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what - is going to fail when we pass in a percentage ("p") requirement. - """ - if "npm install" in arg: - raise BuildFailure('Subprocess return code: 50') - else: - return +def fail_on_npm_install(arg): + """ + For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what + is going to fail when we pass in a percentage ("p") requirement. + """ + if "npm install" in arg: + raise BuildFailure('Subprocess return code: 1') + else: + return + + +def unexpected_fail_on_npm_install(arg): + """ + For our tests, we need the call for diff-quality running pep8 reports to fail, since that is what + is going to fail when we pass in a percentage ("p") requirement. + """ + if "npm install" in arg: + raise BuildFailure('Subprocess return code: 50') + else: + return