From dd6da796ff06ac87a7a8958c178f45665f42f1b6 Mon Sep 17 00:00:00 2001 From: David Manthey Date: Fri, 27 Jan 2017 13:49:41 -0500 Subject: [PATCH 1/3] Improve some errors and some map coverage. Throw errors, not strings. Increase tests for the map class. The map.fileReader function was documented in the geoJSON example as taking a fileReader instance, but it actually took a string and called createFileReader. This function now can take either and has tests. Change where coverage reports are written and collated from. The web-gl and examples tests both use firefox, and their coverage clobbered each other. We aren't using the examples tests to measure coverage, but rather than allow this to happen, change the coverage directories so that they no longer conflict. Otherwise, if the tests were run in the order ff->examples->combine coverage, we would get erroneous coverage information. --- karma-cov.conf.js | 19 +++++++++++++++---- src/d3/d3Renderer.js | 2 +- src/fileReader.js | 2 +- src/map.js | 35 ++++++++++++++++++++--------------- src/ui/widget.js | 4 ++-- tests/cases/map.js | 39 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 78 insertions(+), 23 deletions(-) diff --git a/karma-cov.conf.js b/karma-cov.conf.js index 35f525ee14..8ce9b22fee 100644 --- a/karma-cov.conf.js +++ b/karma-cov.conf.js @@ -10,16 +10,27 @@ function browser(b) { return b.toLowerCase().split(/[ /-]/)[0]; } +function subdir_name(b) { + var subdir = browser(b); + if (process.env.GEOJS_TEST_CASE) { + var parts = /^(.+\/)*(([^\/]+)\.[^\/.]*|[^\/.]+)$/.exec(process.env.GEOJS_TEST_CASE); + if (parts) { + subdir += '_' + (parts[3] || parts[2]); + } + } + return subdir; +} + module.exports = function (config) { var karma_config = require('./karma-base')(config); karma_config.reporters = ['progress', 'coverage']; karma_config.coverageReporter = { reporters: [ - {type: 'html', dir: 'dist/coverage/', subdir: browser}, - {type: 'cobertura', dir: 'dist/cobertura/', file: 'coverage.xml', subdir: browser}, - {type: 'json', dir: 'dist/coverage/json/', subdir: browser}, - {type: 'lcovonly', dir: 'lcov', subdir: browser}, + {type: 'html', dir: 'dist/coverage/', subdir: subdir_name}, + {type: 'cobertura', dir: 'dist/cobertura/', file: 'coverage.xml', subdir: subdir_name}, + {type: 'json', dir: 'dist/coverage/json/', subdir: subdir_name}, + {type: 'lcovonly', dir: 'lcov', subdir: subdir_name}, {type: 'text'} ] }; diff --git a/src/d3/d3Renderer.js b/src/d3/d3Renderer.js index be94e58a26..160a9d5b17 100644 --- a/src/d3/d3Renderer.js +++ b/src/d3/d3Renderer.js @@ -185,7 +185,7 @@ var d3Renderer = function (arg) { m_width = width; m_height = height; if (!m_width || !m_height) { - throw 'Map layer has size 0'; + throw new Error('Map layer has size 0'); } m_diagonal = Math.pow(width * width + height * height, 0.5); m_corners = { diff --git a/src/fileReader.js b/src/fileReader.js index 8eff3c19c6..41a4adb5fd 100644 --- a/src/fileReader.js +++ b/src/fileReader.js @@ -26,7 +26,7 @@ var fileReader = function (arg) { arg = arg || {}; if (!(arg.layer instanceof featureLayer)) { - throw 'fileReader must be given a feature layer'; + throw new Error('fileReader must be given a feature layer'); } var m_layer = arg.layer; diff --git a/src/map.js b/src/map.js index 8fcdda06a2..8889d3c15e 100644 --- a/src/map.js +++ b/src/map.js @@ -860,26 +860,31 @@ var map = function (arg) { //////////////////////////////////////////////////////////////////////////// /** - * Attach a file reader to a layer in the map to be used as a drop target. + * Get, set, or create and set a file reader to a layer in the map to be used + * as a drop target. + * + * @param {string|object|undefined} readerOrName: undefined to get the + * current reader, an instance of a file reader to set the reader, or a + * name to create a file reader (see utils.createFileReader for options). + * @param {object} opts: options for creating a file reader. If this + * includes layer, use that layer, otherwise create a layer using these + * options. */ //////////////////////////////////////////////////////////////////////////// - this.fileReader = function (readerType, opts) { - var layer, renderer; - opts = opts || {}; - if (!readerType) { + this.fileReader = function (readerOrName, opts) { + if (readerOrName === undefined) { return m_fileReader; } - layer = opts.layer; - if (!layer) { - renderer = opts.renderer; - if (!renderer) { - renderer = 'd3'; + if (typeof readerOrName === 'string') { + opts = opts || {}; + if (!opts.layer) { + opts.layer = m_this.createLayer('feature', $.extend({}, opts)); } - layer = m_this.createLayer('feature', {renderer: renderer}); + opts.renderer = opts.layer.renderer().api(); + m_fileReader = registry.createFileReader(readerOrName, opts); + } else { + m_fileReader = readerOrName; } - opts.layer = layer; - opts.renderer = renderer; - m_fileReader = registry.createFileReader(readerType, opts); return m_this; }; @@ -891,7 +896,7 @@ var map = function (arg) { this._init = function () { if (m_node === undefined || m_node === null) { - throw 'Map require DIV node'; + throw new Error('Map require DIV node'); } m_node.addClass('geojs-map'); diff --git a/src/ui/widget.js b/src/ui/widget.js index 0859c4fa5b..0ce3911e1c 100644 --- a/src/ui/widget.js +++ b/src/ui/widget.js @@ -28,7 +28,7 @@ var widget = function (arg) { arg.position = arg.position === undefined ? { left: 0, top: 0 } : arg.position; if (arg.parent !== undefined && !(arg.parent instanceof widget)) { - throw 'Parent must be of type geo.gui.widget'; + throw new Error('Parent must be of type geo.gui.widget'); } this._init = function () { @@ -88,7 +88,7 @@ var widget = function (arg) { */ //////////////////////////////////////////////////////////////////////////// this._createCanvas = function () { - throw 'Must be defined in derived classes'; + throw new Error('Must be defined in derived classes'); }; //////////////////////////////////////////////////////////////////////////// diff --git a/tests/cases/map.js b/tests/cases/map.js index d470526a9d..906dfa74b3 100644 --- a/tests/cases/map.js +++ b/tests/cases/map.js @@ -265,6 +265,20 @@ describe('geo.core.map', function () { m.rotation(17); expect(m.rotation()).toBeCloseTo(17 - Math.PI * 4); }); + it('fileReader', function () { + var m = create_map(); + expect(m.fileReader()).toBe(null); + var layerCount = m.layers().length; + expect(m.fileReader('jsonReader')).toBe(m); + expect(m.fileReader()).not.toBe(null); + expect(m.layers().length).toBe(layerCount + 1); + expect(m.layers()[m.layers().length - 1].renderer().api()).not.toBe('d3'); + expect(m.fileReader('jsonReader', {renderer: 'd3'})).toBe(m); + expect(m.layers()[m.layers().length - 1].renderer().api()).toBe('d3'); + var r = geo.createFileReader('jsonReader', {layer: m.layers()[m.layers().length - 1]}); + expect(m.fileReader(r)).toBe(m); + expect(m.fileReader()).toBe(r); + }); }); describe('Public utility methods', function () { @@ -584,5 +598,30 @@ describe('geo.core.map', function () { $(window).trigger('resize'); expect(m.size()).toEqual({width: 400, height: 400}); }); + it('dragover', function () { + var m = create_map(); + var evt = $.Event('dragover'); + evt.originalEvent = new window.Event('dragover'); + evt.originalEvent.dataTransfer = {}; + $(m.node()).trigger(evt); + expect(evt.originalEvent.dataTransfer.dropEffect).not.toBe('copy'); + m.fileReader('jsonReader'); + evt = $.Event('dragover'); + evt.originalEvent = new window.Event('dragover'); + evt.originalEvent.dataTransfer = {}; + $(m.node()).trigger(evt); + expect(evt.originalEvent.dataTransfer.dropEffect).toBe('copy'); + }); + it('drop', function () { + var m = create_map(); + m.fileReader('jsonReader', {renderer: 'd3'}); + var evt = $.Event('drop'); + evt.originalEvent = new window.Event('drop'); + evt.originalEvent.dataTransfer = {files: [{ + geometry: {coordinates: [1, 2], type: 'Point'}, type: 'Feature' + }]}; + $(m.node()).trigger(evt); + expect(m.layers()[0].features().length).toBe(1); + }); }); }); From 8de604fd44482053c2e7853b6b6e57a7fca1e12a Mon Sep 17 00:00:00 2001 From: David Manthey Date: Wed, 1 Feb 2017 09:35:15 -0500 Subject: [PATCH 2/3] Add comments about what the regexps are doing. --- karma-cov.conf.js | 5 +++++ tests/cases/mapInteractor.js | 3 +++ 2 files changed, 8 insertions(+) diff --git a/karma-cov.conf.js b/karma-cov.conf.js index 8ce9b22fee..6c1ed3a0cb 100644 --- a/karma-cov.conf.js +++ b/karma-cov.conf.js @@ -7,12 +7,17 @@ var path = require('path'); * Return URL friendly browser string */ function browser(b) { + /* The browser string starts with the basic browser name (Firefox, PhantomJS, + * etc. Split on the first space or dash to isolate this name. */ return b.toLowerCase().split(/[ /-]/)[0]; } function subdir_name(b) { var subdir = browser(b); if (process.env.GEOJS_TEST_CASE) { + /* Use thes test case and as part of the name so that different tests end + * up in different coverage directories. Get the last file name without + * extenskon from the test case for this purpose. */ var parts = /^(.+\/)*(([^\/]+)\.[^\/.]*|[^\/.]+)$/.exec(process.env.GEOJS_TEST_CASE); if (parts) { subdir += '_' + (parts[3] || parts[2]); diff --git a/tests/cases/mapInteractor.js b/tests/cases/mapInteractor.js index 133dbd5659..3eec5ef266 100644 --- a/tests/cases/mapInteractor.js +++ b/tests/cases/mapInteractor.js @@ -1682,4 +1682,7 @@ describe('mapInteractor', function () { expect(interactor.keyboard(keyboardSettings)).toBe(interactor); expect(map.node().hasClass('highlight-focus')).toBe(true); }); + it('Test touch interaction', function () { + //DWM:: + }); }); From 03638a6b8cd791003f901f25cbea914fc4e92adf Mon Sep 17 00:00:00 2001 From: David Manthey Date: Wed, 1 Feb 2017 12:52:46 -0500 Subject: [PATCH 3/3] Fix a typo and some wording. --- karma-cov.conf.js | 4 ++-- tests/cases/mapInteractor.js | 3 --- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/karma-cov.conf.js b/karma-cov.conf.js index 6c1ed3a0cb..da4f3c8f4d 100644 --- a/karma-cov.conf.js +++ b/karma-cov.conf.js @@ -16,8 +16,8 @@ function subdir_name(b) { var subdir = browser(b); if (process.env.GEOJS_TEST_CASE) { /* Use thes test case and as part of the name so that different tests end - * up in different coverage directories. Get the last file name without - * extenskon from the test case for this purpose. */ + * up in different coverage directories. Get the last element of the path + * without the extension from the test case for this purpose. */ var parts = /^(.+\/)*(([^\/]+)\.[^\/.]*|[^\/.]+)$/.exec(process.env.GEOJS_TEST_CASE); if (parts) { subdir += '_' + (parts[3] || parts[2]); diff --git a/tests/cases/mapInteractor.js b/tests/cases/mapInteractor.js index 3eec5ef266..133dbd5659 100644 --- a/tests/cases/mapInteractor.js +++ b/tests/cases/mapInteractor.js @@ -1682,7 +1682,4 @@ describe('mapInteractor', function () { expect(interactor.keyboard(keyboardSettings)).toBe(interactor); expect(map.node().hasClass('highlight-focus')).toBe(true); }); - it('Test touch interaction', function () { - //DWM:: - }); });