From e2cab190847706f19de181c0eedf9cfcb559fad6 Mon Sep 17 00:00:00 2001 From: stv Date: Wed, 4 Jun 2014 17:56:10 -0700 Subject: [PATCH 1/5] Ensure JSInput tests actually run This test currently fails, meaning that the existing tests weren't testing what they claimed. Assertions are made for each element returned by the CSS selectors. However, the selectors are assumed to be wildcard matches, but are actually literal selectors. As there are no matched elements, this causes the assertions to be (silently) checked zero times, without failure. --- common/static/js/capa/spec/jsinput_spec.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/common/static/js/capa/spec/jsinput_spec.js b/common/static/js/capa/spec/jsinput_spec.js index b857972fb4..fb043e1631 100644 --- a/common/static/js/capa/spec/jsinput_spec.js +++ b/common/static/js/capa/spec/jsinput_spec.js @@ -27,4 +27,12 @@ describe("JSInput", function() { expect(inputField.data('waitfor')).toBeDefined(); }); }); + + it('tests the correct number of sections', function () { + var sections = $(document).find('section[id="inputtype_"]'); + var inputFields = $(document).find('input[id="input_"]'); + expect(sections.length).toEqual(2); + expect(sections.length).toEqual(inputFields.length); + }); }); + From 60a9e998715e303f3c9d09970099cc171f3b4143 Mon Sep 17 00:00:00 2001 From: stv Date: Wed, 4 Jun 2014 18:41:35 -0700 Subject: [PATCH 2/5] Fix JSInput tests Select DOM elements with wildcard syntax - DOM lookups were being done with non-existent literal selectors, so it was returning empty lists. As assertions were to be made while iterating over the list of elements, nothing was actually being verified. - Common code has been centralized in the setup function. By declaring CSS selectors once, we minimize the odds of inadvertently using the wrong selector, as happened here. - Had these tests actually been iterating over a non-empty list, this would have thrown undefined exceptions. jQuery.each calls its handler with an index and an item/object as the arguments. However, the object is a DOM-object, not a jQuery-object. These tests break, as they had assumed the latter. --- common/static/js/capa/spec/jsinput_spec.js | 30 +++++++++++----------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/common/static/js/capa/spec/jsinput_spec.js b/common/static/js/capa/spec/jsinput_spec.js index fb043e1631..c54265ca7d 100644 --- a/common/static/js/capa/spec/jsinput_spec.js +++ b/common/static/js/capa/spec/jsinput_spec.js @@ -1,36 +1,36 @@ -describe("JSInput", function() { +describe("JSInput", function () { + var sections; + var inputFields; + beforeEach(function () { loadFixtures('js/capa/fixtures/jsinput.html'); + sections = $('section[id^="inputtype_"]'); + inputFields = $('input[id^="input_"]'); + JSInput.walkDOM(); }); - it('sets all data-processed attributes to true on first load', function() { - var sections = $(document).find('section[id="inputtype_"]'); - JSInput.walkDOM(); - sections.each(function(index, section) { - expect(section.attr('data-processed')).toEqual('true'); + it('sets all data-processed attributes to true on first load', function () { + sections.each(function (index, item) { + expect(item).toHaveData('processed', true); }); }); it('sets the data-processed attribute to true on subsequent load', function() { - var section1 = $(document).find('section[id="inputtype_1"]'), - section2 = $(document).find('section[id="inputtype_2"]'); + var section1 = $(this.sections[0]); + var section2 = $(this.sections[1]); section1.attr('data-processed', false); JSInput.walkDOM(); expect(section1.attr('data-processed')).toEqual('true'); expect(section2.attr('data-processed')).toEqual('true'); }); - it('sets the waitfor attribute to its update function', function() { - var inputFields = $(document).find('input[id="input_"]'); - JSInput.walkDOM(); - inputFields.each(function(index, inputField) { - expect(inputField.data('waitfor')).toBeDefined(); + it('sets the waitfor attribute to its update function', function () { + inputFields.each(function (index, item) { + expect(item).toHaveAttr('waitfor'); }); }); it('tests the correct number of sections', function () { - var sections = $(document).find('section[id="inputtype_"]'); - var inputFields = $(document).find('input[id="input_"]'); expect(sections.length).toEqual(2); expect(sections.length).toEqual(inputFields.length); }); From bdf90bfcfbdc88b143dc0a4fbe4ea56ca8acecdf Mon Sep 17 00:00:00 2001 From: stv Date: Thu, 5 Jun 2014 10:00:59 -0700 Subject: [PATCH 3/5] Remove superfluous JSInput test The behavior previously tested here was to check that if a JSInput element was marked as not processed, re-walking the DOM *should* have reinitialized it. Unfortunately, this behavior is not supported by the underlying JSChannel library. In fact, if JSChannel detects an existing channel with the same origin and scope, it throws an uncaught exception, leaving the DOM in a "broken" state. JSInput will prevent duplicates from being added, as long as we don't manually update the `data-processed` attribute. This behavior is already being tested. --- common/static/js/capa/spec/jsinput_spec.js | 9 --------- 1 file changed, 9 deletions(-) diff --git a/common/static/js/capa/spec/jsinput_spec.js b/common/static/js/capa/spec/jsinput_spec.js index c54265ca7d..b62097b72c 100644 --- a/common/static/js/capa/spec/jsinput_spec.js +++ b/common/static/js/capa/spec/jsinput_spec.js @@ -15,15 +15,6 @@ describe("JSInput", function () { }); }); - it('sets the data-processed attribute to true on subsequent load', function() { - var section1 = $(this.sections[0]); - var section2 = $(this.sections[1]); - section1.attr('data-processed', false); - JSInput.walkDOM(); - expect(section1.attr('data-processed')).toEqual('true'); - expect(section2.attr('data-processed')).toEqual('true'); - }); - it('sets the waitfor attribute to its update function', function () { inputFields.each(function (index, item) { expect(item).toHaveAttr('waitfor'); From cf14dee79006d3448b8ea07ee18a35f1e96c8a05 Mon Sep 17 00:00:00 2001 From: stv Date: Thu, 5 Jun 2014 12:49:34 -0700 Subject: [PATCH 4/5] Fix JSInput test fixture Clean up fixtures automatically between tests - Nesting test fixture markup within a DIV allows Jasmine to automatically restore the fixture to a clean state between each test run. Fix id attribute collision typo - This looks like a copy/pasta gone wrong; the two test INPUT elements were both declared with the same id, `input_1`. Remove trailing whitespace --- common/static/js/capa/fixtures/jsinput.html | 31 +++++++++++---------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/common/static/js/capa/fixtures/jsinput.html b/common/static/js/capa/fixtures/jsinput.html index 87feaaaffe..1862d490c3 100644 --- a/common/static/js/capa/fixtures/jsinput.html +++ b/common/static/js/capa/fixtures/jsinput.html @@ -1,5 +1,6 @@ +
- +
- \ No newline at end of file + + + From fd10fbf725756a12a8c57b8b42fb9a0c086dcf08 Mon Sep 17 00:00:00 2001 From: stv Date: Thu, 5 Jun 2014 12:41:18 -0700 Subject: [PATCH 5/5] Fix JSInput scope leak Declare variables locally - The `allSections` variable was leaking into the global scope, due to a typo in the declaration. - `dataProcessed` can be declared more narrowly within the callback. --- common/static/js/capa/src/jsinput.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/common/static/js/capa/src/jsinput.js b/common/static/js/capa/src/jsinput.js index a10f18fa76..a1ad7e8bc7 100644 --- a/common/static/js/capa/src/jsinput.js +++ b/common/static/js/capa/src/jsinput.js @@ -181,19 +181,15 @@ var JSInput = (function ($, undefined) { } function walkDOM() { - var dataProcessed, all; - - // Find all jsinput elements - allSections = $('section.jsinput'); - + var allSections = $('section.jsinput'); // When a JSInput problem loads, its data-processed attribute is false, // so the jsconstructor will be called for it. // The constructor will not be called again on subsequent reruns of // this file by other JSInput. Only if it is reloaded, either with the // rest of the page or when it is submitted, will this constructor be - // called again. + // called again. allSections.each(function(index, value) { - dataProcessed = ($(value).attr("data-processed") === "true"); + var dataProcessed = ($(value).attr("data-processed") === "true"); if (!dataProcessed) { jsinputConstructor(value); $(value).attr("data-processed", 'true');