Fix issues with Underscore in the asset pipeline

FEDX-121

The previous approach for handling NPM assets was
to symlink them into the static directory. This appeared
to cause trouble with the asset pipeline where the files
in question were not installed and then old versions were
picked up instead.

This change instead copies NPM libraries to a new
static directory so that the pipeline can consume them
as with any other file. This new directory is added to
.gitignore so that the files don't get accidentally
checked in.

.gitignore

@@ -73,6 +73,7 @@ bin/
 lms/static/css/
 lms/static/certificates/css/
 cms/static/css/
+common/static/common/js/vendor/
 
 ### Styling generated from templates
 lms/static/sass/*.css

cms/static/cms/js/require-config.js

@@ -50,7 +50,7 @@
             "moment": "js/vendor/moment.min",
             "moment-with-locales": "js/vendor/moment-with-locales.min",
             "text": 'js/vendor/requirejs/text',
-            "underscore": "js/vendor/underscore-min",
+            "underscore": "common/js/vendor/underscore",
             "underscore.string": "js/vendor/underscore.string.min",
             "backbone": "js/vendor/backbone-min",
             "backbone-relational" : "js/vendor/backbone-relational.min",

cms/static/coffee/spec/main.coffee

@@ -26,7 +26,7 @@ requirejs.config({
         "moment": "xmodule_js/common_static/js/vendor/moment.min",
         "moment-with-locales": "xmodule_js/common_static/js/vendor/moment-with-locales.min",
         "text": "xmodule_js/common_static/js/vendor/requirejs/text",
-        "underscore": "xmodule_js/common_static/js/vendor/underscore-min",
+        "underscore": "xmodule_js/common_static/common/js/vendor/underscore",
         "underscore.string": "xmodule_js/common_static/js/vendor/underscore.string.min",
         "backbone": "xmodule_js/common_static/js/vendor/backbone-min",
         "backbone.associations": "xmodule_js/common_static/js/vendor/backbone-associations-min",

cms/static/coffee/spec/main_squire.coffee

@@ -22,7 +22,7 @@ requirejs.config({
         "datepair": "xmodule_js/common_static/js/vendor/timepicker/datepair",
         "date": "xmodule_js/common_static/js/vendor/date",
         "text": "xmodule_js/common_static/js/vendor/requirejs/text",
-        "underscore": "xmodule_js/common_static/js/vendor/underscore-min",
+        "underscore": "xmodule_js/common_static/common/js/vendor/underscore",
         "underscore.string": "xmodule_js/common_static/js/vendor/underscore.string.min",
         "backbone": "xmodule_js/common_static/js/vendor/backbone-min",
         "backbone.associations": "xmodule_js/common_static/js/vendor/backbone-associations-min",

cms/static/js_test.yml

@@ -35,7 +35,7 @@ lib_paths:
     - xmodule_js/common_static/js/vendor/jquery-ui.min.js
     - xmodule_js/common_static/js/vendor/jquery.cookie.js
     - xmodule_js/common_static/js/vendor/jquery.simulate.js
-    - xmodule_js/common_static/js/vendor/underscore-min.js
+    - xmodule_js/common_static/common/js/vendor/underscore.js
     - xmodule_js/common_static/js/vendor/underscore.string.min.js
     - xmodule_js/common_static/js/vendor/backbone-min.js
     - xmodule_js/common_static/js/vendor/backbone-associations-min.js

cms/static/js_test_squire.yml

@@ -34,7 +34,7 @@ lib_paths:
     - xmodule_js/common_static/js/vendor/jquery.min.js
     - xmodule_js/common_static/js/vendor/jquery-ui.min.js
     - xmodule_js/common_static/js/vendor/jquery.cookie.js
-    - xmodule_js/common_static/js/vendor/underscore-min.js
+    - xmodule_js/common_static/common/js/vendor/underscore.js
     - xmodule_js/common_static/js/vendor/underscore.string.min.js
     - xmodule_js/common_static/js/vendor/backbone-min.js
     - xmodule_js/common_static/js/vendor/backbone-associations-min.js

common/lib/xmodule/xmodule/js/js_test.yml

@@ -45,7 +45,7 @@ lib_paths:
     - common_static/js/vendor/jquery.ui.draggable.js
     - common_static/js/vendor/jquery.cookie.js
     - common_static/js/vendor/json2.js
-    - common_static/js/vendor/underscore-min.js
+    - common_static/common/js/vendor/underscore.js
     - common_static/js/vendor/backbone-min.js
     - common_static/js/vendor/jquery.leanModal.js
     - common_static/js/vendor/CodeMirror/codemirror.js

common/static/common/js/spec/main_requirejs.js

@@ -22,7 +22,7 @@
             'jquery.url': 'js/vendor/url.min',
             'sinon': 'js/vendor/sinon-1.17.0',
             'text': 'js/vendor/requirejs/text',
-            'underscore': 'js/vendor/underscore-min',
+            'underscore': 'common/js/vendor/underscore',
             'underscore.string': 'js/vendor/underscore.string.min',
             'backbone': 'js/vendor/backbone-min',
             'backbone.associations': 'js/vendor/backbone-associations-min',

common/static/js/vendor/underscore-min.js

-../../../../node_modules/underscore/underscore-min.js
\ No newline at end of file

common/static/js_test.yml

@@ -33,7 +33,7 @@ lib_paths:
     - js/vendor/jasmine-imagediff.js
     - js/vendor/jquery.truncate.js
     - js/vendor/mustache.js
-    - js/vendor/underscore-min.js
+    - common/js/vendor/underscore.js
     - js/vendor/underscore.string.min.js
     - js/vendor/backbone-min.js
     - js/vendor/jquery.timeago.js

lms/envs/common.py

@@ -1242,7 +1242,7 @@ base_vendor_js = [
     'js/vendor/jquery.min.js',
     'js/vendor/jquery.cookie.js',
     'js/vendor/url.min.js',
-    'js/vendor/underscore-min.js',
+    'common/js/vendor/underscore.js',
     'js/vendor/underscore.string.min.js',
     'js/vendor/requirejs/require.js',
     'js/RequireJS-namespace-undefine.js',

lms/static/js/spec/main.js

@@ -29,7 +29,7 @@
             'moment': 'xmodule_js/common_static/js/vendor/moment.min',
             'moment-with-locales': 'xmodule_js/common_static/js/vendor/moment-with-locales.min',
             'text': 'xmodule_js/common_static/js/vendor/requirejs/text',
-            'underscore': 'xmodule_js/common_static/js/vendor/underscore-min',
+            'underscore': 'xmodule_js/common_static/common/js/vendor/underscore',
             'underscore.string': 'xmodule_js/common_static/js/vendor/underscore.string.min',
             'backbone': 'xmodule_js/common_static/js/vendor/backbone-min',
             'backbone.associations': 'xmodule_js/common_static/js/vendor/backbone-associations-min',

lms/static/js_test.yml

@@ -56,7 +56,7 @@ lib_paths:
     - xmodule_js/src/video/
     - xmodule_js/src/xmodule.js
     - xmodule_js/common_static/js/src/
-    - xmodule_js/common_static/js/vendor/underscore-min.js
+    - xmodule_js/common_static/common/js/vendor/underscore.js
     - xmodule_js/common_static/js/vendor/underscore.string.min.js
     - xmodule_js/common_static/js/vendor/backbone-min.js
     - xmodule_js/common_static/js/vendor/backbone.paginator.min.js

lms/static/lms/js/require-config.js

@@ -47,7 +47,7 @@
             "backbone": "js/vendor/backbone-min",
             "backbone-super": "js/vendor/backbone-super",
             "backbone.paginator": "js/vendor/backbone.paginator.min",
-            "underscore": "js/vendor/underscore-min",
+            "underscore": "common/js/vendor/underscore",
             "underscore.string": "js/vendor/underscore.string.min",
             "jquery": "js/vendor/jquery.min",
             "jquery.cookie": "js/vendor/jquery.cookie",

pavelib/assets.py

@@ -35,6 +35,15 @@ CMS_SASS_DIRECTORIES = [
 THEME_SASS_DIRECTORIES = []
 SASS_LOAD_PATHS = ['common/static', 'common/static/sass']
 
+# A list of NPM installed libraries that should be copied into the common
+# static directory.
+NPM_INSTALLED_LIBRARIES = [
+    'underscore/underscore.js'
+]
+
+# Directory to install static vendor files
+NPM_VENDOR_DIRECTORY = path("common/static/common/js/vendor")
+
 
 def configure_paths():
     """Configure our paths based on settings.  Called immediately."""
@@ -292,6 +301,26 @@ def compile_templated_sass(systems, settings):
         print("\t\tFinished preprocessing {} assets.".format(system))
 
 
+def process_npm_assets():
+    """
+    Process vendor libraries installed via NPM.
+    """
+    # Skip processing of the libraries if this is just a dry run
+    if tasks.environment.dry_run:
+        tasks.environment.info("install npm_assets")
+        return
+
+    # Ensure that the vendor directory exists
+    NPM_VENDOR_DIRECTORY.mkdir_p()
+
+    # Copy each file to the vendor directory, overwriting any existing file.
+    for library in NPM_INSTALLED_LIBRARIES:
+        sh('/bin/cp -rf node_modules/{library} {vendor_dir}'.format(
+            library=library,
+            vendor_dir=NPM_VENDOR_DIRECTORY,
+        ))
+
+
 def process_xmodule_assets():
     """
     Process XModule static assets.
@@ -387,6 +416,7 @@ def update_assets(args):
 
     compile_templated_sass(args.system, args.settings)
     process_xmodule_assets()
+    process_npm_assets()
     compile_coffeescript()
     call_task('pavelib.assets.compile_sass', options={'system': args.system, 'debug': args.debug})
 

pavelib/paver_tests/test_js_test.py

+"""Unit tests for the Paver JavaScript testing tasks."""
+
+import ddt
+from mock import patch
+from paver.easy import call_task
+
+import pavelib.js_test
+from .utils import PaverTestCase
+
+
+@ddt.ddt
+class TestPaverJavaScriptTestTasks(PaverTestCase):
+    """
+    Test the Paver JavaScript testing tasks.
+    """
+
+    EXPECTED_DELETE_JAVASCRIPT_REPORT_COMMAND = u'find {platform_root}/reports/javascript -type f -delete'
+    EXPECTED_INSTALL_NPM_ASSETS_COMMAND = u'install npm_assets'
+    EXPECTED_COFFEE_COMMAND = (
+        u'node_modules/.bin/coffee --compile `find {platform_root}/lms {platform_root}/cms '
+        u'{platform_root}/common -type f -name "*.coffee"`'
+    )
+    EXPECTED_JS_TEST_TOOL_OPTIONS = (
+        u"{platform_root}/lms/static/js_test.yml "
+        u"{platform_root}/lms/static/js_test_coffee.yml "
+        u"{platform_root}/cms/static/js_test.yml "
+        u"{platform_root}/cms/static/js_test_squire.yml "
+        u"{platform_root}/common/lib/xmodule/xmodule/js/js_test.yml "
+        u"{platform_root}/common/static/js_test.yml "
+        u"{platform_root}/common/static/js_test_requirejs.yml "
+        u"--use-firefox "
+        u"--timeout-sec 600 "
+        u"--xunit-report "
+        u"{platform_root}/reports/javascript/javascript_xunit.xml"
+    )
+    EXPECTED_COVERAGE_OPTIONS = (
+        u' --coverage-xml {platform_root}/reports/javascript/coverage.xml'
+    )
+
+    EXPECTED_COMMANDS = [
+        u"make report_dir",
+        u'git clean -fqdx test_root/logs test_root/data test_root/staticfiles test_root/uploads',
+        u"find . -name '.git' -prune -o -name '*.pyc' -exec rm {} \\;",
+        u'rm -rf test_root/log/auto_screenshots/*',
+        u"rm -rf /tmp/mako_[cl]ms",
+    ]
+
+    def setUp(self):
+        super(TestPaverJavaScriptTestTasks, self).setUp()
+
+        # Mock the paver @needs decorator
+        self._mock_paver_needs = patch.object(pavelib.js_test.test_js, 'needs').start()
+        self._mock_paver_needs.return_value = 0
+
+        # Cleanup mocks
+        self.addCleanup(self._mock_paver_needs.stop)
+
+    @ddt.data(
+        [""],
+        ["--coverage"],
+        ["--suite=lms"],
+        ["--suite=lms --coverage"],
+    )
+    @ddt.unpack
+    def test_test_js_run(self, options_string):
+        """
+        Test the "test_js_run" task.
+        """
+        options = self.parse_options_string(options_string)
+        self.reset_task_messages()
+        call_task("pavelib.js_test.test_js_run", options=options)
+        self.verify_messages(options=options, dev_mode=False)
+
+    @ddt.data(
+        [""],
+        ["--port=9999"],
+        ["--suite=lms"],
+        ["--suite=lms --port=9999"],
+    )
+    @ddt.unpack
+    def test_test_js_dev(self, options_string):
+        """
+        Test the "test_js_run" task.
+        """
+        options = self.parse_options_string(options_string)
+        self.reset_task_messages()
+        call_task("pavelib.js_test.test_js_dev", options=options)
+        self.verify_messages(options=options, dev_mode=True)
+
+    def parse_options_string(self, options_string):
+        """
+        Parse a string containing the options for a test run
+        """
+        parameters = options_string.split(" ")
+        suite = "all"
+        if "--system=lms" in parameters:
+            suite = "lms"
+        elif "--system=common" in parameters:
+            suite = "common"
+        coverage = "--coverage" in parameters
+        port = None
+        if "--port=9999" in parameters:
+            port = 9999
+        return {
+            "suite": suite,
+            "coverage": coverage,
+            "port": port,
+        }
+
+    def verify_messages(self, options, dev_mode):
+        """
+        Verify that the messages generated when running tests are as expected
+        for the specified options and dev_mode.
+        """
+        is_coverage = options['coverage']
+        port = options['port']
+        expected_messages = []
+        expected_messages.extend(self.EXPECTED_COMMANDS)
+        if not dev_mode and not is_coverage:
+            expected_messages.append(self.EXPECTED_DELETE_JAVASCRIPT_REPORT_COMMAND.format(
+                platform_root=self.platform_root
+            ))
+        expected_messages.append(self.EXPECTED_INSTALL_NPM_ASSETS_COMMAND)
+        expected_messages.append(self.EXPECTED_COFFEE_COMMAND.format(platform_root=self.platform_root))
+        expected_test_tool_command = u'js-test-tool {command} {options}'.format(
+            command='dev' if dev_mode else 'run',
+            options=self.EXPECTED_JS_TEST_TOOL_OPTIONS.format(platform_root=self.platform_root),
+        )
+        if is_coverage:
+            expected_test_tool_command += self.EXPECTED_COVERAGE_OPTIONS.format(platform_root=self.platform_root)
+        if port:
+            expected_test_tool_command += u" -p {port}".format(port=port)
+        expected_messages.append(expected_test_tool_command)
+        self.assertEquals(self.task_messages, expected_messages)

pavelib/paver_tests/test_servers.py

 """Unit tests for the Paver server tasks."""
 
 import ddt
-import os
 from paver.easy import call_task
 
 from .utils import PaverTestCase
 
 EXPECTED_COFFEE_COMMAND = (
-    "node_modules/.bin/coffee --compile `find {platform_root}/lms "
-    "{platform_root}/cms {platform_root}/common -type f -name \"*.coffee\"`"
+    u"node_modules/.bin/coffee --compile `find {platform_root}/lms "
+    u"{platform_root}/cms {platform_root}/common -type f -name \"*.coffee\"`"
 )
 EXPECTED_SASS_COMMAND = (
-    "libsass {sass_directory}"
+    u"libsass {sass_directory}"
 )
 EXPECTED_COMMON_SASS_DIRECTORIES = [
-    "common/static/sass",
+    u"common/static/sass",
 ]
 EXPECTED_LMS_SASS_DIRECTORIES = [
-    "lms/static/sass",
-    "lms/static/themed_sass",
-    "lms/static/certificates/sass",
+    u"lms/static/sass",
+    u"lms/static/themed_sass",
+    u"lms/static/certificates/sass",
 ]
 EXPECTED_CMS_SASS_DIRECTORIES = [
-    "cms/static/sass",
+    u"cms/static/sass",
 ]
 EXPECTED_PREPROCESS_ASSETS_COMMAND = (
-    "python manage.py {system} --settings={asset_settings} preprocess_assets"
-    " {system}/static/sass/*.scss {system}/static/themed_sass"
+    u"python manage.py {system} --settings={asset_settings} preprocess_assets"
+    u" {system}/static/sass/*.scss {system}/static/themed_sass"
 )
 EXPECTED_COLLECT_STATIC_COMMAND = (
-    "python manage.py {system} --settings={asset_settings} collectstatic --noinput > /dev/null"
+    u"python manage.py {system} --settings={asset_settings} collectstatic --noinput > /dev/null"
 )
 EXPECTED_CELERY_COMMAND = (
-    "python manage.py lms --settings={settings} celery worker --beat --loglevel=INFO --pythonpath=."
+    u"python manage.py lms --settings={settings} celery worker --beat --loglevel=INFO --pythonpath=."
 )
 EXPECTED_RUN_SERVER_COMMAND = (
-    "python manage.py {system} --settings={settings} runserver --traceback --pythonpath=. 0.0.0.0:{port}"
+    u"python manage.py {system} --settings={settings} runserver --traceback --pythonpath=. 0.0.0.0:{port}"
 )
 EXPECTED_INDEX_COURSE_COMMAND = (
-    "python manage.py {system} --settings={settings} reindex_course --setup"
+    u"python manage.py {system} --settings={settings} reindex_course --setup"
 )
 
 
@@ -227,13 +226,13 @@ class TestPaverServerTasks(PaverTestCase):
             expected_settings = "devstack_optimized"
             expected_asset_settings = "test_static_optimized"
         expected_collect_static = not is_fast and expected_settings != "devstack"
-        platform_root = os.getcwd()
         if not is_fast:
             expected_messages.append(EXPECTED_PREPROCESS_ASSETS_COMMAND.format(
                 system=system, asset_settings=expected_asset_settings
             ))
-            expected_messages.append("xmodule_assets common/static/xmodule")
-            expected_messages.append(EXPECTED_COFFEE_COMMAND.format(platform_root=platform_root))
+            expected_messages.append(u"xmodule_assets common/static/xmodule")
+            expected_messages.append(u"install npm_assets")
+            expected_messages.append(EXPECTED_COFFEE_COMMAND.format(platform_root=self.platform_root))
             expected_messages.extend(self.expected_sass_commands(system=system))
         if expected_collect_static:
             expected_messages.append(EXPECTED_COLLECT_STATIC_COMMAND.format(
@@ -265,7 +264,6 @@ class TestPaverServerTasks(PaverTestCase):
             expected_settings = "devstack_optimized"
             expected_asset_settings = "test_static_optimized"
         expected_collect_static = not is_fast and expected_settings != "devstack"
-        platform_root = os.getcwd()
         expected_messages = []
         if not is_fast:
             expected_messages.append(EXPECTED_PREPROCESS_ASSETS_COMMAND.format(
@@ -274,8 +272,9 @@ class TestPaverServerTasks(PaverTestCase):
             expected_messages.append(EXPECTED_PREPROCESS_ASSETS_COMMAND.format(
                 system="cms", asset_settings=expected_asset_settings
             ))
-            expected_messages.append("xmodule_assets common/static/xmodule")
-            expected_messages.append(EXPECTED_COFFEE_COMMAND.format(platform_root=platform_root))
+            expected_messages.append(u"xmodule_assets common/static/xmodule")
+            expected_messages.append(u"install npm_assets")
+            expected_messages.append(EXPECTED_COFFEE_COMMAND.format(platform_root=self.platform_root))
             expected_messages.extend(self.expected_sass_commands())
         if expected_collect_static:
             expected_messages.append(EXPECTED_COLLECT_STATIC_COMMAND.format(

pavelib/paver_tests/utils.py

@@ -31,6 +31,11 @@ class PaverTestCase(TestCase):
         """Returns the messages output by the Paver task."""
         return tasks.environment.messages
 
+    @property
+    def platform_root(self):
+        """Returns the current platform's root directory."""
+        return os.getcwd()
+
     def reset_task_messages(self):
         """Clear the recorded message"""
         tasks.environment.messages = []
@@ -52,4 +57,4 @@ class MockEnvironment(tasks.Environment):
         else:
             output = message
         if not output.startswith("--->"):
-            self.messages.append(output)
+            self.messages.append(unicode(output))

pavelib/utils/test/suites/js_suite.py

 """
 Javascript test tasks
 """
+
+from paver import tasks
+
 from pavelib import assets
 from pavelib.utils.test import utils as test_utils
 from pavelib.utils.test.suites.suite import TestSuite
@@ -31,13 +34,17 @@ class JsTestSuite(TestSuite):
 
     def __enter__(self):
         super(JsTestSuite, self).__enter__()
-        self.report_dir.makedirs_p()
+        if tasks.environment.dry_run:
+            tasks.environment.info("make report_dir")
+        else:
+            self.report_dir.makedirs_p()
         if not self.skip_clean:
             test_utils.clean_test_files()
 
         if self.mode == 'run' and not self.run_under_coverage:
             test_utils.clean_dir(self.report_dir)
 
+        assets.process_npm_assets()
         assets.compile_coffeescript("`find lms cms common -type f -name \"*.coffee\"`")
 
     @property

pavelib/utils/test/suites/suite.py

@@ -3,6 +3,8 @@ A class used for defining and running test suites
 """
 import sys
 import subprocess
+
+from paver import tasks
 from paver.easy import sh
 
 from pavelib.utils.process import kill_process
@@ -74,6 +76,11 @@ class TestSuite(object):
         returns True.
         """
         cmd = self.cmd
+
+        if tasks.environment.dry_run:
+            tasks.environment.info(cmd)
+            return
+
         sys.stdout.write(cmd)
 
         msg = colorize(
@@ -130,6 +137,10 @@ class TestSuite(object):
         Runs the tests in the suite while tracking and reporting failures.
         """
         self.run_suite_tests()
+
+        if tasks.environment.dry_run:
+            return
+
         self.report_test_results()
 
         if len(self.failed_suites) > 0: