Merge branch 'master' into jonahstanley/fix-flakey-tests

Conflicts:
	common/djangoapps/terrain/ui_helpers.py

AUTHORS

@@ -77,3 +77,4 @@ Slater Victoroff <slater.r.victoroff@gmail.com>
 Peter Fogg <peter.p.fogg@gmail.com>
 Bethany LaPenta <lapentab@mit.edu>
 Renzo Lucioni <renzolucioni@gmail.com>
+Felix Sun <felixsun@mit.edu>

CHANGELOG.rst

@@ -5,6 +5,19 @@ These are notable changes in edx-platform.  This is a rolling list of changes,
 in roughly chronological order, most recent first.  Add your entries at or near
 the top.  Include a label indicating the component affected.
 
+XModule: Only write out assets files if the contents have changed.
+
+XModule: Don't delete generated xmodule asset files when compiling (for
+instance, when XModule provides a coffeescript file, don't delete
+the associated javascript)
+
+Common: Make asset watchers run as singletons (so they won't start if the
+watcher is already running in another shell).
+
+Common: Use coffee directly when watching for coffeescript file changes.
+
+Common: Make rake provide better error messages if packages are missing.
+
 Common: Repairs development documentation generation by sphinx.
 
 LMS: Problem rescoring.  Added options on the Grades tab of the
@@ -14,6 +27,8 @@ students' number of attempts to zero.  Provides a list of background
 tasks that are currently running for the course, and an option to
 see a history of background tasks for a given problem.
 
+LMS: Fixed the preferences scope for storing data in xmodules.
+
 LMS: Forums.  Added handling for case where discussion module can get `None` as
 value of lms.start in `lms/djangoapps/django_comment_client/utils.py`
 

Gemfile

@@ -4,3 +4,4 @@ gem 'sass', '3.1.15'
 gem 'bourbon', '~> 1.3.6'
 gem 'colorize', '~> 0.5.8'
 gem 'launchy', '~> 2.1.2'
+gem 'sys-proctable', '~> 0.9.3'

cms/djangoapps/contentstore/features/advanced-settings.py

@@ -31,11 +31,10 @@ def press_the_notification_button(step, name):
 
     # Save was clicked if either the save notification bar is gone, or we have a error notification
     # overlaying it (expected in the case of typing Object into display_name).
-    save_clicked = lambda : world.is_css_not_present('.is-shown.wrapper-notification-warning') or \
-        world.is_css_present('.is-shown.wrapper-notification-error')
+    save_clicked = lambda: world.is_css_not_present('.is-shown.wrapper-notification-warning') or\
+                           world.is_css_present('.is-shown.wrapper-notification-error')
 
-    assert_true(world.css_click(css, success_condition=save_clicked),
-        'The save button was not clicked after 5 attempts.')
+    assert_true(world.css_click(css, success_condition=save_clicked), 'Save button not clicked after 5 attempts.')
 
 
 @step(u'I edit the value of a policy key$')

common/djangoapps/terrain/ui_helpers.py

@@ -49,7 +49,7 @@ def css_has_text(css_selector, text):
 
 @world.absorb
 def css_find(css, wait_time=5):
-    def is_visible(driver):
+    def is_visible(_driver):
         return EC.visibility_of_element_located((By.CSS_SELECTOR, css,))
 
     world.browser.is_element_present_by_css(css, wait_time=wait_time)
@@ -58,7 +58,7 @@ def css_find(css, wait_time=5):
 
 
 @world.absorb
-def css_click(css_selector, index=0, attempts=5, success_condition=lambda:True):
+def css_click(css_selector, index=0, attempts=5, success_condition=lambda: True):
     """
     Perform a click on a CSS selector, retrying if it initially fails.
 
@@ -122,15 +122,15 @@ def css_check(css_selector, index=0, attempts=5, success_condition=lambda: True)
 
 
 @world.absorb
-def css_click_at(css, x=10, y=10):
+def css_click_at(css, x_cord=10, y_cord=10):
     '''
     A method to click at x,y coordinates of the element
     rather than in the center of the element
     '''
-    e = css_find(css).first
-    e.action_chains.move_to_element_with_offset(e._element, x, y)
-    e.action_chains.click()
-    e.action_chains.perform()
+    element = css_find(css).first
+    element.action_chains.move_to_element_with_offset(element._element, x_cord, y_cord)
+    element.action_chains.click()
+    element.action_chains.perform()
 
 
 @world.absorb
@@ -175,7 +175,7 @@ def css_visible(css_selector):
 
 @world.absorb
 def dialogs_closed():
-    def are_dialogs_closed(driver):
+    def are_dialogs_closed(_driver):
         '''
         Return True when no modal dialogs are visible
         '''
@@ -186,12 +186,12 @@ def dialogs_closed():
 
 @world.absorb
 def save_the_html(path='/tmp'):
-    u = world.browser.url
+    url = world.browser.url
     html = world.browser.html.encode('ascii', 'ignore')
-    filename = '%s.html' % quote_plus(u)
-    f = open('%s/%s' % (path, filename), 'w')
-    f.write(html)
-    f.close()
+    filename = '%s.html' % quote_plus(url)
+    file = open('%s/%s' % (path, filename), 'w')
+    file.write(html)
+    file.close()
 
 
 @world.absorb

common/lib/xmodule/xmodule/static_content.py

@@ -4,6 +4,7 @@ This module has utility functions for gathering up the static content
 that is defined by XModules and XModuleDescriptors (javascript and css)
 """
 
+import logging
 import hashlib
 import os
 import errno
@@ -15,6 +16,9 @@ from path import path
 from xmodule.x_module import XModuleDescriptor
 
 
+LOG = logging.getLogger(__name__)
+
+
 def write_module_styles(output_root):
     return _write_styles('.xmodule_display', output_root, _list_modules())
 
@@ -121,18 +125,32 @@ def _write_js(output_root, classes):
             type=filetype)
         contents[filename] = fragment
 
-    _write_files(output_root, contents)
+    _write_files(output_root, contents, {'.coffee': '.js'})
 
     return [output_root / filename for filename in contents.keys()]
 
 
-def _write_files(output_root, contents):
+def _write_files(output_root, contents, generated_suffix_map=None):
     _ensure_dir(output_root)
-    for extra_file in set(output_root.files()) - set(contents.keys()):
-        extra_file.remove_p()
+    to_delete = set(file.basename() for file in output_root.files()) - set(contents.keys())
+
+    if generated_suffix_map:
+        for output_file in contents.keys():
+            for suffix, generated_suffix in generated_suffix_map.items():
+                if output_file.endswith(suffix):
+                    to_delete.discard(output_file.replace(suffix, generated_suffix))
+
+    for extra_file in to_delete:
+        (output_root / extra_file).remove_p()
 
     for filename, file_content in contents.iteritems():
-        (output_root / filename).write_bytes(file_content)
+        output_file = output_root / filename
+
+        if not output_file.isfile() or output_file.read_md5() != hashlib.md5(file_content).digest():
+            LOG.debug("Writing %s", output_file)
+            output_file.write_bytes(file_content)
+        else:
+            LOG.debug("%s unchanged, skipping", output_file)
 
 
 def main():

doc/development.md

@@ -63,6 +63,25 @@ To get a full list of available rake tasks, use:
 
     rake -T
 
+### Troubleshooting
+
+#### Reference Error: XModule is not defined (javascript)
+This means that the javascript defining an xmodule hasn't loaded correctly. There are a number
+of different things that could be causing this:
+
+1. See `Error: watch EMFILE`
+
+#### Error: watch EMFILE (coffee)
+When running a development server, we also start a watcher process alongside to recompile coffeescript
+and sass as changes are made. On Mac OSX systems, the coffee watcher process takes more file handles
+than are allowed by default. This will result in `EMFILE` errors when coffeescript is running, and
+will prevent javascript from compiling, leading to the error 'XModule is not defined'
+
+To work around this issue, we use `Process::setrlimit` to set the number of allowed open files.
+Coffee watches both directories and files, so you will need to set this fairly high (anecdotally,
+8000 seems to do the trick on OSX 10.7.5, 10.8.3, and 10.8.4)
+
+
 ## Running Tests
 
 See `testing.md` for instructions on running the test suite.

lms/djangoapps/courseware/features/video.feature

 Feature: Video component
   As a student, I want to view course videos in LMS.
 
-  Scenario: Autoplay is enabled in LMS
-    Given the course has a Video component
-    Then when I view the video it has autoplay enabled
+  Scenario: Autoplay is enabled in LMS for a Video component
+  Given the course has a Video component
+  Then when I view the video it has autoplay enabled
+
+  Scenario: Autoplay is enabled in the LMS for a VideoAlpha component
+  Given the course has a VideoAlpha component
+  Then when I view the video it has autoplay enabled

lms/djangoapps/courseware/features/video.py

@@ -27,8 +27,30 @@ def view_video(_step):
     world.browser.visit(url)
 
 
+@step('the course has a VideoAlpha component')
+def view_videoalpha(step):
+    coursename = TEST_COURSE_NAME.replace(' ', '_')
+    i_am_registered_for_the_course(step, coursename)
+
+    # Make sure we have a videoalpha
+    add_videoalpha_to_course(coursename)
+    chapter_name = TEST_SECTION_NAME.replace(" ", "_")
+    section_name = chapter_name
+    url = django_url('/courses/edx/Test_Course/Test_Course/courseware/%s/%s' %
+                     (chapter_name, section_name))
+
+    world.browser.visit(url)
+
+
 def add_video_to_course(course):
     template_name = 'i4x://edx/templates/video/default'
     world.ItemFactory.create(parent_location=section_location(course),
                              template=template_name,
                              display_name='Video')
+
+
+def add_videoalpha_to_course(course):
+    template_name = 'i4x://edx/templates/videoalpha/Video_Alpha'
+    world.ItemFactory.create(parent_location=section_location(course),
+                             template=template_name,
+                             display_name='Video Alpha')

lms/djangoapps/courseware/features/videoalpha.feature

-Feature: Video Alpha component
-  As a student, I want to view course videos in LMS.
-
-  Scenario: Autoplay is enabled in LMS
-    Given the course has a Video component
-    Then when I view the video it has autoplay enabled

lms/djangoapps/courseware/features/videoalpha.py

-#pylint: disable=C0111
-#pylint: disable=W0613
-#pylint: disable=W0621
-
-from lettuce import world, step
-from lettuce.django import django_url
-from common import TEST_COURSE_NAME, TEST_SECTION_NAME, i_am_registered_for_the_course, section_location
-
-############### ACTIONS ####################
-
-
-@step('when I view the video it has autoplay enabled')
-def does_autoplay(step):
-    assert(world.css_find('.videoalpha')[0]['data-autoplay'] == 'True')
-
-
-@step('the course has a Video component')
-def view_videoalpha(step):
-    coursename = TEST_COURSE_NAME.replace(' ', '_')
-    i_am_registered_for_the_course(step, coursename)
-
-    # Make sure we have a videoalpha
-    add_videoalpha_to_course(coursename)
-    chapter_name = TEST_SECTION_NAME.replace(" ", "_")
-    section_name = chapter_name
-    url = django_url('/courses/edx/Test_Course/Test_Course/courseware/%s/%s' %
-                     (chapter_name, section_name))
-
-    world.browser.visit(url)
-
-
-def add_videoalpha_to_course(course):
-    template_name = 'i4x://edx/templates/videoalpha/default'
-    world.ItemFactory.create(parent_location=section_location(course),
-                             template=template_name,
-                             display_name='Video Alpha 1')

lms/djangoapps/courseware/model_data.py

@@ -163,7 +163,7 @@ class ModelDataCache(object):
             return self._chunked_query(
                 XModuleStudentPrefsField,
                 'module_type__in',
-                set(descriptor.location.category for descriptor in self.descriptors),
+                set(descriptor.module_class.__name__ for descriptor in self.descriptors),
                 student=self.user.pk,
                 field_name__in=set(field.name for field in fields),
             )

lms/djangoapps/courseware/tests/factories.py

@@ -75,7 +75,7 @@ class StudentPrefsFactory(DjangoModelFactory):
     field_name = 'existing_field'
     value = json.dumps('old_value')
     student = SubFactory(UserFactory)
-    module_type = 'problem'
+    module_type = 'MockProblemModule'
 
 
 class StudentInfoFactory(DjangoModelFactory):

lms/djangoapps/courseware/tests/test_model_data.py

@@ -29,6 +29,7 @@ def mock_descriptor(fields=[], lms_fields=[]):
     descriptor.location = location('def_id')
     descriptor.module_class.fields = fields
     descriptor.module_class.lms.fields = lms_fields
+    descriptor.module_class.__name__ = 'MockProblemModule'
     return descriptor
 
 location = partial(Location, 'i4x', 'edX', 'test_course', 'problem')
@@ -37,7 +38,7 @@ course_id = 'edX/test_course/test'
 content_key = partial(LmsKeyValueStore.Key, Scope.content, None, location('def_id'))
 settings_key = partial(LmsKeyValueStore.Key, Scope.settings, None, location('def_id'))
 user_state_key = partial(LmsKeyValueStore.Key, Scope.user_state, 'user', location('def_id'))
-prefs_key = partial(LmsKeyValueStore.Key, Scope.preferences, 'user', 'problem')
+prefs_key = partial(LmsKeyValueStore.Key, Scope.preferences, 'user', 'MockProblemModule')
 user_info_key = partial(LmsKeyValueStore.Key, Scope.user_info, 'user', None)
 
 
@@ -190,6 +191,10 @@ class StorageTestBase(object):
         self.mdc = ModelDataCache([mock_descriptor([mock_field(self.scope, 'existing_field')])], course_id, self.user)
         self.kvs = LmsKeyValueStore(self.desc_md, self.mdc)
 
+    def test_set_and_get_existing_field(self):
+        self.kvs.set(self.key_factory('existing_field'), 'test_value')
+        self.assertEquals('test_value', self.kvs.get(self.key_factory('existing_field')))
+
     def test_get_existing_field(self):
         "Test that getting an existing field in an existing Storage Field works"
         self.assertEquals('old_value', self.kvs.get(self.key_factory('existing_field')))

lms/templates/video.html

@@ -2,37 +2,33 @@
   <h2> ${display_name} </h2>
 % endif
 
-%if settings.MITX_FEATURES['STUB_VIDEO_FOR_TESTING']:
-  <div id="stub_out_video_for_testing">
-    <div class="video" data-autoplay="${settings.MITX_FEATURES['AUTOPLAY_VIDEOS']}">
-      <section class="video-controls">
-	<div class="slider"></div>
-	<div>
-	  <ul class="vcr">
-            <li><a class="video_control" href="#"></a></li>
-            <li>
-              <div class="vidtime">0:00 / 0:00</div>
-            </li>
-	  </ul>
-	  <div class="secondary-controls">
-            <a href="#" class="add-fullscreen" title="Fill browser">Fill Browser</a>
-	  </div>
-	</div>
-      </section>
-    </div>
-  </div>
-%elif settings.MITX_FEATURES.get('USE_YOUTUBE_OBJECT_API') and normal_speed_video_id:
+%if settings.MITX_FEATURES.get('USE_YOUTUBE_OBJECT_API') and normal_speed_video_id:
   <object width="640" height="390">
     <param name="movie"
-           value="https://www.youtube.com/v/${normal_speed_video_id}?version=3&amp;autoplay=1&amp;rel=0"></param>
+    % if not settings.MITX_FEATURES['STUB_VIDEO_FOR_TESTING']:
+    value="https://www.youtube.com/v/${normal_speed_video_id}?version=3&amp;autoplay=1&amp;rel=0">
+    % endif
+    </param>
     <param name="allowScriptAccess" value="always"></param>
-    <embed src="https://www.youtube.com/v/${normal_speed_video_id}?version=3&amp;autoplay=1&amp;rel=0"
-           type="application/x-shockwave-flash"
-           allowscriptaccess="always"
-           width="640" height="390"></embed>
+    <embed 
+       % if not settings.MITX_FEATURES['STUB_VIDEO_FOR_TESTING']:
+       src="https://www.youtube.com/v/${normal_speed_video_id}?version=3&amp;autoplay=1&amp;rel=0"
+       % endif
+       type="application/x-shockwave-flash"
+       allowscriptaccess="always"
+       width="640" height="390"></embed>
   </object>
 %else:
-  <div id="video_${id}" class="video" data-streams="${streams}" data-show-captions="${show_captions}" data-start="${start}" data-end="${end}" data-caption-asset-path="${caption_asset_path}" data-autoplay="${settings.MITX_FEATURES['AUTOPLAY_VIDEOS']}">
+<div id="video_${id}" class="video" 
+
+    % if not settings.MITX_FEATURES['STUB_VIDEO_FOR_TESTING']:
+    data-streams="${streams}" 
+    % endif
+
+    data-show-captions="${show_captions}" 
+    data-start="${start}" data-end="${end}" 
+    data-caption-asset-path="${caption_asset_path}" 
+    data-autoplay="${settings.MITX_FEATURES['AUTOPLAY_VIDEOS']}">
     <div class="tc-wrapper">
       <article class="video-wrapper">
         <section class="video-player">

lms/templates/videoalpha.html

@@ -2,34 +2,38 @@
   <h2> ${display_name} </h2>
 % endif
 
-%if settings.MITX_FEATURES['STUB_VIDEO_FOR_TESTING']:
-  <div id="stub_out_video_for_testing"></div>
-%else:
-  <div
-      id="video_${id}"
-      class="video"
-      data-streams="${youtube_streams}"
-      ${'data-sub="{}"'.format(sub) if sub else ''}
-      ${'data-mp4-source="{}"'.format(sources.get('mp4')) if sources.get('mp4') else ''}
-      ${'data-webm-source="{}"'.format(sources.get('webm')) if sources.get('webm') else ''}
-      ${'data-ogg-source="{}"'.format(sources.get('ogv')) if sources.get('ogv') else ''}
-      data-caption-data-dir="${data_dir}"
-      data-show-captions="${show_captions}"
-      data-start="${start}"
-      data-end="${end}"
-      data-caption-asset-path="${caption_asset_path}"
-      data-autoplay="${autoplay}"
-  >
-    <div class="tc-wrapper">
-      <article class="video-wrapper">
-        <section class="video-player">
-          <div id="${id}"></div>
-        </section>
-        <section class="video-controls"></section>
-      </article>
-    </div>
-  </div>
-%endif
+<div
+  id="video_${id}"
+  class="video"
+  
+  % if not settings.MITX_FEATURES['STUB_VIDEO_FOR_TESTING']:
+  data-streams="${youtube_streams}"
+  % endif
+
+  ${'data-sub="{}"'.format(sub) if sub else ''}
+
+  % if not settings.MITX_FEATURES['STUB_VIDEO_FOR_TESTING']:
+  ${'data-mp4-source="{}"'.format(sources.get('mp4')) if sources.get('mp4') else ''}
+  ${'data-webm-source="{}"'.format(sources.get('webm')) if sources.get('webm') else ''}
+  ${'data-ogg-source="{}"'.format(sources.get('ogv')) if sources.get('ogv') else ''}
+  % endif
+
+  data-caption-data-dir="${data_dir}"
+  data-show-captions="${show_captions}"
+  data-start="${start}"
+  data-end="${end}"
+  data-caption-asset-path="${caption_asset_path}"
+  data-autoplay="${autoplay}"
+>
+<div class="tc-wrapper">
+  <article class="video-wrapper">
+    <section class="video-player">
+      <div id="${id}"></div>
+    </section>
+    <section class="video-controls"></section>
+  </article>
+</div>
+</div>
 
 % if sources.get('main'):
 <div class="video-sources">

rakefile

-require 'json'
-require 'rake/clean'
-require './rakefiles/helpers.rb'
-
-Dir['rakefiles/*.rake'].each do |rakefile|
-    import rakefile
+begin
+    require 'json'
+    require 'rake/clean'
+    require './rakelib/helpers.rb'
+rescue LoadError => error
+    puts "Import faild (#{error})"
+    puts "Please run `bundle install` to bootstrap ruby dependencies"
+    exit 1
 end
 
 # Build Constants

rakefiles/assets.rake → rakelib/assets.rake

@@ -6,6 +6,8 @@ if USE_CUSTOM_THEME
     THEME_SASS = File.join(THEME_ROOT, "static", "sass")
 end
 
+MINIMAL_DARWIN_NOFILE_LIMIT = 8000
+
 def xmodule_cmd(watch=false, debug=false)
     xmodule_cmd = 'xmodule_assets common/static/xmodule'
     if watch
@@ -21,24 +23,14 @@ def xmodule_cmd(watch=false, debug=false)
 end
 
 def coffee_cmd(watch=false, debug=false)
-    if watch
-        # On OSx, coffee fails with EMFILE when
-        # trying to watch all of our coffee files at the same
-        # time.
-        #
-        # Ref: https://github.com/joyent/node/issues/2479
-        #
-        # So, instead, we use watchmedo, which works around the problem
-        "watchmedo shell-command " +
-                  "--command 'node_modules/.bin/coffee -c ${watch_src_path}' " +
-                  "--recursive " +
-                  "--patterns '*.coffee' " +
-                  "--ignore-directories " +
-                  "--wait " +
-                  "."
-    else
-        'node_modules/.bin/coffee --compile .'
+    if watch && Launchy::Application.new.host_os_family.darwin?
+        available_files = Process::getrlimit(:NOFILE)[0]
+        if available_files < MINIMAL_DARWIN_NOFILE_LIMIT
+            Process.setrlimit(:NOFILE, MINIMAL_DARWIN_NOFILE_LIMIT)
+
+        end
     end
+    "node_modules/.bin/coffee --compile #{watch ? '--watch' : ''} ."
 end
 
 def sass_cmd(watch=false, debug=false)
@@ -55,8 +47,9 @@ def sass_cmd(watch=false, debug=false)
           "#{watch ? '--watch' : '--update'} -E utf-8 #{sass_watch_paths.join(' ')}"
 end
 
+# This task takes arguments purely to pass them via dependencies to the preprocess task
 desc "Compile all assets"
-multitask :assets => 'assets:all'
+task :assets, [:system, :env] => 'assets:all'
 
 namespace :assets do
 
@@ -80,8 +73,9 @@ namespace :assets do
     {:xmodule => [:install_python_prereqs],
      :coffee => [:install_node_prereqs, :'assets:coffee:clobber'],
      :sass => [:install_ruby_prereqs, :preprocess]}.each_pair do |asset_type, prereq_tasks|
+        # This task takes arguments purely to pass them via dependencies to the preprocess task
         desc "Compile all #{asset_type} assets"
-        task asset_type => prereq_tasks do
+        task asset_type, [:system, :env] => prereq_tasks do |t, args|
             cmd = send(asset_type.to_s + "_cmd", watch=false, debug=false)
             if cmd.kind_of?(Array)
                 cmd.each {|c| sh(c)}
@@ -90,7 +84,8 @@ namespace :assets do
             end
         end
 
-        multitask :all => asset_type
+        # This task takes arguments purely to pass them via dependencies to the preprocess task
+        multitask :all, [:system, :env] => asset_type
         multitask :debug => "assets:#{asset_type}:debug"
         multitask :_watch => "assets:#{asset_type}:_watch"
 
@@ -111,9 +106,9 @@ namespace :assets do
             task :_watch => (prereq_tasks + ["assets:#{asset_type}:debug"]) do
                 cmd = send(asset_type.to_s + "_cmd", watch=true, debug=true)
                 if cmd.kind_of?(Array)
-                    cmd.each {|c| background_process(c)}
+                    cmd.each {|c| singleton_process(c)}
                 else
-                    background_process(cmd)
+                    singleton_process(cmd)
                 end
             end
         end

rakefiles/helpers.rb → rakelib/helpers.rb

 require 'digest/md5'
+require 'sys/proctable'
+require 'colorize'
 
 def find_executable(exec)
     path = %x(which #{exec}).strip
@@ -84,6 +86,16 @@ def background_process(*command)
     end
 end
 
+# Runs a command as a background process, as long as no other processes
+# tagged with the same tag are running
+def singleton_process(*command)
+    if Sys::ProcTable.ps.select {|proc| proc.cmdline.include?(command.join(' '))}.empty?
+        background_process(*command)
+    else
+        puts "Process '#{command.join(' ')} already running, skipping".blue
+    end
+end
+
 def environments(system)
     Dir["#{system}/envs/**/*.py"].select{|file| ! (/__init__.py$/ =~ file)}.map do |env_file|
         env_file.gsub("#{system}/envs/", '').gsub(/\.py/, '').gsub('/', '.')

rakefiles/prereqs.rake → rakelib/prereqs.rake

-require './rakefiles/helpers.rb'
-
 PREREQS_MD5_DIR = ENV["PREREQ_CACHE_DIR"] || File.join(REPO_ROOT, '.prereqs_cache')
 
 CLOBBER.include(PREREQS_MD5_DIR)