From 835b3af17a9c3cf3fde8e0219a9c3444f959362e Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Fri, 3 Jan 2014 17:10:44 -0300 Subject: [PATCH 01/17] Bare async provider implementation. Not working due to an issue in code inspection implementation (html template is displayed before it's rendered). --- src/language/CodeInspection.js | 59 ++++++++++++++++++++++------------ 1 file changed, 39 insertions(+), 20 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 26309e0be85..0187c92349c 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -170,29 +170,48 @@ define(function (require, exports, module) { response.resolve(null); return response.promise(); } - + DocumentManager.getDocumentText(file) .done(function (fileText) { - var perfTimerInspector = PerfUtils.markStart("CodeInspection:\t" + file.fullPath); - - providerList.forEach(function (provider) { - var perfTimerProvider = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + file.fullPath); - - try { - var scanResult = provider.scanFile(fileText, file.fullPath); - results.push({provider: provider, result: scanResult}); - } catch (err) { - console.error("[CodeInspection] Provider " + provider.name + " threw an error: " + err); - response.reject(err); - return; + var perfTimerInspector = PerfUtils.markStart("CodeInspection:\t" + file.fullPath), + masterPromise; + + masterPromise = Async.doInParallel(providerList, function (provider) { + var perfTimerProvider = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + file.fullPath), + runPromise = new $.Deferred(); + + if (provider.scanFileAsync) { + provider.scanFileAsync(fileText, file.fullPath) + .then(function (scanResult) { + results.push({provider: provider, result: scanResult}); + PerfUtils.addMeasurement(perfTimerProvider); + runPromise.resolve(); + }) + .fail(function (err) { + console.error("[CodeInpsection] Provider " + provider.name + " (async) failed: " + err); + runPromise.reject(err); + }); + } else { + try { + var scanResult = provider.scanFile(fileText, file.fullPath); + results.push({provider: provider, result: scanResult}); + PerfUtils.addMeasurement(perfTimerProvider); + runPromise.resolve(); + } catch (err) { + console.error("[CodeInspection] Provider " + provider.name + " (sync) threw an error: " + err); + runPromise.reject(err); + return; + } } + return runPromise.promise(); - PerfUtils.addMeasurement(perfTimerProvider); + }, false); + + masterPromise.then(function () { + PerfUtils.addMeasurement(perfTimerInspector); + response.resolve(results); }); - PerfUtils.addMeasurement(perfTimerInspector); - - response.resolve(results); }) .fail(function (err) { console.error("[CodeInspection] Could not read file for inspection: " + file.fullPath); @@ -328,7 +347,7 @@ define(function (require, exports, module) { html = Mustache.render(ResultsTemplate, {reportList: allErrors}); }); - + // Update results table var perfTimerDOM = PerfUtils.markStart("ProblemsPanel render:\t" + currentDoc.file.fullPath); @@ -377,7 +396,7 @@ define(function (require, exports, module) { if (!_providers[languageId]) { _providers[languageId] = []; } - + if (languageId === "javascript") { // This is a special case to enable extension provider to replace the JSLint provider // in favor of their own implementation @@ -385,7 +404,7 @@ define(function (require, exports, module) { return registeredProvider.name === "JSLint"; }); } - + _providers[languageId].push(provider); run(); From e730595539e822d6d9816e61f271a0e4b6471378 Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Sun, 12 Jan 2014 18:39:00 -0300 Subject: [PATCH 02/17] Fix @param specification error. --- src/language/CodeInspection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index d62befe9f76..7405302cda9 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -394,7 +394,7 @@ define(function (require, exports, module) { * registered providers. * * @param {string} languageId - * @param {{name:string, scanFile:function(string, string):?{errors:!Array, aborted:boolean}} provider + * @param {{name:string, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}} provider * * Each error is: { pos:{line,ch}, endPos:?{line,ch}, message:string, type:?Type } * If type is unspecified, Type.WARNING is assumed. From d9deaa1e3f22dd7a79db76c61534615c1bc8103d Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Wed, 15 Jan 2014 11:49:38 -0300 Subject: [PATCH 03/17] Change documentation and JSDoc. --- src/language/CodeInspection.js | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 7405302cda9..d542b77b6d4 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -59,7 +59,7 @@ define(function (require, exports, module) { Async = require("utils/Async"), PanelTemplate = require("text!htmlContent/problems-panel.html"), ResultsTemplate = require("text!htmlContent/problems-panel-table.html"); - + var INDICATOR_ID = "status-inspection", defaultPrefs = { enabled: brackets.config["linting.enabled_by_default"], @@ -117,7 +117,7 @@ define(function (require, exports, module) { /** * @private - * @type {Object.>} + * @type {Object.>} */ var _providers = {}; @@ -145,7 +145,7 @@ define(function (require, exports, module) { * Decision is made depending on the file extension. * * @param {!string} filePath - * @return ?{Array.<{name:string, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}>} provider + * @return ?{Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}>} provider */ function getProvidersForPath(filePath) { return _providers[LanguageManager.getLanguageForPath(filePath).getId()]; @@ -157,13 +157,20 @@ define(function (require, exports, module) { * This method doesn't update the Brackets UI, just provides inspection results. * These results will reflect any unsaved changes present in the file if currently open. * + * If a provider implements scanFileAsync, then it will be used to scan the file. Otherwise, scanFile, + * a synchronous version of the function will be used. Provider must never reject a promise and resolve it + * with null in case the results cannot be retrieved for whatever reason. + * + * A code inspection provider's scanFileAsync must return a {$.Promise} object which must be resolved with + * be resolved with ?{errors:!Array, aborted:boolean}}. + * * The Promise yields an array of provider-result pair objects (the result is the return value of the * provider's scanFile() - see register() for details). The result object may be null if there were no * errors from that provider. * If there are no providers registered for this file, the Promise yields null instead. * * @param {!File} file File that will be inspected for errors. - * @param {?Array.<{name:string, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}>} providerList + * @param {?Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}>} providerList * @return {$.Promise} a jQuery promise that will be resolved with ?Array.<{provider:Object, result: ?{errors:!Array, aborted:boolean}}> */ function inspectFile(file, providerList) { @@ -232,7 +239,7 @@ define(function (require, exports, module) { * change based on the number of problems reported and how many provider reported problems. * * @param {Number} numProblems - total number of problems across all providers - * @param {Array.<{name:string, scanFile:function(string, string):Object}>} providersReportingProblems - providers that reported problems + * @param {Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:function(string, string):Object}>} providersReportingProblems - providers that reported problems * @param {boolean} aborted - true if any provider returned a result with the 'aborted' flag set */ function updatePanelTitleAndStatusBar(numProblems, providersReportingProblems, aborted) { @@ -394,7 +401,7 @@ define(function (require, exports, module) { * registered providers. * * @param {string} languageId - * @param {{name:string, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}} provider + * @param {{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}} provider * * Each error is: { pos:{line,ch}, endPos:?{line,ch}, message:string, type:?Type } * If type is unspecified, Type.WARNING is assumed. From b91ec3f7849233ff7355a03137e475eb4d96bfc0 Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Wed, 15 Jan 2014 12:21:50 -0300 Subject: [PATCH 04/17] Put timeout on provider execution. Use runPromise as the aggregation point instead of doing it in varios places. --- src/language/CodeInspection.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index d542b77b6d4..079c5d93777 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -193,26 +193,29 @@ define(function (require, exports, module) { var perfTimerProvider = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + file.fullPath), runPromise = new $.Deferred(); + runPromise.then(function(scanResult) { + results.push({provider: provider, result: scanResult}); + }); + setTimeout(function () { runPromise.resolve(); }, 500); + if (provider.scanFileAsync) { provider.scanFileAsync(fileText, file.fullPath) .then(function (scanResult) { - results.push({provider: provider, result: scanResult}); PerfUtils.addMeasurement(perfTimerProvider); - runPromise.resolve(); + runPromise.resolve(scanResult); }) .fail(function (err) { console.error("[CodeInspection] Provider " + provider.name + " (async) failed: " + err); - runPromise.reject(err); + runPromise.resolve(null); }); } else { try { var scanResult = provider.scanFile(fileText, file.fullPath); - results.push({provider: provider, result: scanResult}); PerfUtils.addMeasurement(perfTimerProvider); - runPromise.resolve(); + runPromise.resolve(scanResult); } catch (err) { console.error("[CodeInspection] Provider " + provider.name + " (sync) threw an error: " + err); - runPromise.reject(err); + runPromise.resolve(null); return; } } From 08e0392efd96789acbccc7e6d2ec516e8efe1c6a Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Wed, 15 Jan 2014 12:28:19 -0300 Subject: [PATCH 05/17] Change to record notation (understood by CC). --- src/language/CodeInspection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 079c5d93777..c7e3ea1200c 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -117,7 +117,7 @@ define(function (require, exports, module) { /** * @private - * @type {Object.>} + * @type {{languageId:string, Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:function(string, string):Object}>}} */ var _providers = {}; From 03c9ed3ca0fbe2a76dd0f4ca0e3b2aa0f80adab5 Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Wed, 15 Jan 2014 14:23:54 -0300 Subject: [PATCH 06/17] Fix typo in the docs. --- src/language/CodeInspection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index c7e3ea1200c..a951460d709 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -162,7 +162,7 @@ define(function (require, exports, module) { * with null in case the results cannot be retrieved for whatever reason. * * A code inspection provider's scanFileAsync must return a {$.Promise} object which must be resolved with - * be resolved with ?{errors:!Array, aborted:boolean}}. + * ?{errors:!Array, aborted:boolean}}. * * The Promise yields an array of provider-result pair objects (the result is the return value of the * provider's scanFile() - see register() for details). The result object may be null if there were no From 5ec55346793b8639e27e1bf33dfdc04b3e385706 Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Thu, 16 Jan 2014 20:50:49 -0300 Subject: [PATCH 07/17] Resolve with null on timeout. Lint. --- src/language/CodeInspection.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index a951460d709..d939cdf0829 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -23,7 +23,7 @@ /*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4 */ -/*global define, $, Mustache, brackets */ +/*global define, $, Mustache, brackets, setTimeout */ /** * Manages linters and other code inspections on a per-language basis. Provides a UI and status indicator for @@ -193,12 +193,12 @@ define(function (require, exports, module) { var perfTimerProvider = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + file.fullPath), runPromise = new $.Deferred(); - runPromise.then(function(scanResult) { + runPromise.then(function (scanResult) { results.push({provider: provider, result: scanResult}); }); - setTimeout(function () { runPromise.resolve(); }, 500); if (provider.scanFileAsync) { + setTimeout(function () { runPromise.resolve(null); }, 500); provider.scanFileAsync(fileText, file.fullPath) .then(function (scanResult) { PerfUtils.addMeasurement(perfTimerProvider); @@ -216,7 +216,6 @@ define(function (require, exports, module) { } catch (err) { console.error("[CodeInspection] Provider " + provider.name + " (sync) threw an error: " + err); runPromise.resolve(null); - return; } } return runPromise.promise(); From 9fc042559f959aa22fa1f9804096bab753a60560 Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Thu, 16 Jan 2014 20:53:07 -0300 Subject: [PATCH 08/17] Add unit tests. --- test/spec/CodeInspection-test.js | 148 +++++++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index 47a512d18b2..764448163fc 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -57,6 +57,29 @@ define(function (require, exports, module) { return provider; } + + function createAsyncCodeInspector(name, result, scanTime, syncImpl) { + var provider = { + name: name, + scanFileAsync: function () { + var deferred = new $.Deferred(); + setTimeout(function () { + deferred.resolve(result); + }, scanTime); + return deferred.promise(); + } + }; + spyOn(provider, "scanFileAsync").andCallThrough(); + + if (syncImpl === true) { + provider.scanFile = function () { + return result; + }; + spyOn(provider, "scanFile").andCallThrough(); + } + + return provider; + } function successfulLintResult() { return {errors: []}; @@ -243,6 +266,131 @@ define(function (require, exports, module) { expect(expectedResult).toBeNull(); }); }); + + it("should run asynchoronous implementation when both available in the provider", function () { + var provider = createAsyncCodeInspector("javascript async linter with sync impl", failLintResult(), 200, true); + CodeInspection.register("javascript", provider); + + runs(function () { + var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); + + waitsForDone(promise, "file linting", 5000); + }); + + runs(function () { + expect(provider.scanFileAsync).toHaveBeenCalled(); + expect(provider.scanFile).not.toHaveBeenCalled(); + }); + + }); + + it("should timeout on a provider that takes too long", function () { + var provider = createAsyncCodeInspector("javascript async linter with sync impl", failLintResult(), 1000, true), + result; + CodeInspection.register("javascript", provider); + + runs(function () { + var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); + promise.done(function (r) { + result = r; + }); + + waitsForDone(promise, "file linting", 550); + }); + + runs(function () { + expect(provider.scanFileAsync).toHaveBeenCalled(); + expect(result).toBeDefined(); + expect(result[0].provider).toEqual(provider); + expect(result[0].errors).toBeFalsy(); + }); + + }); + + it("should run two asynchronous providers and a synchronous one", function () { + var asyncProvider1 = createAsyncCodeInspector("javascript async linter 1", failLintResult(), 200, true), + asyncProvider2 = createAsyncCodeInspector("javascript async linter 2", successfulLintResult(), 300, false), + syncProvider3 = createCodeInspector("javascript sync linter 3", failLintResult()), + result; + CodeInspection.register("javascript", asyncProvider1); + CodeInspection.register("javascript", asyncProvider2); + CodeInspection.register("javascript", syncProvider3); + + runs(function () { + var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); + promise.done(function (r) { + result = r; + }); + + waitsForDone(promise, "file linting", 5000); + }); + + runs(function () { + var i; + expect(result.length).toEqual(3); + + for (i = 0; i < result.length; i++) { + switch (result[i].provider.name) { + case asyncProvider1.name: + expect(asyncProvider1.scanFile).not.toHaveBeenCalled(); + expect(asyncProvider2.scanFileAsync).toHaveBeenCalled(); + break; + case asyncProvider2.name: + expect(asyncProvider2.scanFileAsync).toHaveBeenCalled(); + break; + case syncProvider3.name: + expect(syncProvider3.scanFile).toHaveBeenCalled(); + break; + default: + expect(true).toBe(false); + break; + } + } + }); + + }); + + it("should return results for 3 providers when 2 completes and 1 times out", function () { + var asyncProvider1 = createAsyncCodeInspector("javascript async linter 1", failLintResult(), 200, true), + asyncProvider2 = createAsyncCodeInspector("javascript async linter 2", failLintResult(), 1000, false), + syncProvider3 = createCodeInspector("javascript sync linter 3", failLintResult()), + result; + CodeInspection.register("javascript", asyncProvider1); + CodeInspection.register("javascript", asyncProvider2); + CodeInspection.register("javascript", syncProvider3); + + runs(function () { + var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); + promise.done(function (r) { + result = r; + }); + + waitsForDone(promise, "file linting", 5000); + }); + + runs(function () { + var i; + expect(result.length).toEqual(3); + + for (i = 0; i < result.length; i++) { + switch (result[i].provider.name) { + case asyncProvider1.name: + case syncProvider3.name: + expect(result[i].result).toBeDefined(); + expect(result[i].result).not.toBeNull(); + break; + case asyncProvider2.name: + expect(result[i].result).toBeDefined(); + expect(result[i].result).toBeNull(); + break; + default: + expect(true).toBe(false); + break; + } + } + }); + }); + }); describe("Code Inspection UI", function () { From 0c091df7ade1cfa4ca8f77e08ef3fa8b781eb359 Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Mon, 17 Mar 2014 10:43:02 -0300 Subject: [PATCH 09/17] Address code review comments. --- src/language/CodeInspection.js | 4 ++++ test/spec/CodeInspection-test.js | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 8cf63d8a6f1..d7dae6fd44a 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -408,6 +408,10 @@ define(function (require, exports, module) { * Registering any provider for the "javascript" language automatically unregisters the built-in * Brackets JSLint provider. This is a temporary convenience until UI exists for disabling * registered providers. + * + * If provider implements both scanFileAsync and scanFile functions for asynchronous and synchronous + * code inspection, respectively, the asynchronous version will take precedence and will be used to + * perform code inspection. * * @param {string} languageId * @param {{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}} provider diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index 7eda4563970..255655dc073 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -75,7 +75,7 @@ define(function (require, exports, module) { }; spyOn(provider, "scanFileAsync").andCallThrough(); - if (syncImpl === true) { + if (syncImpl) { provider.scanFile = function () { return result; }; From fe7d3420e3132b5bc72f3f1c039dc0c4aeabd89e Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Mon, 17 Mar 2014 11:46:20 -0300 Subject: [PATCH 10/17] Increase timeout to 1000ms and make it a pref. Adjust the values in the unit tests. --- src/language/CodeInspection.js | 7 ++++--- test/spec/CodeInspection-test.js | 6 +++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index d7dae6fd44a..72490a25bee 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -76,7 +76,8 @@ define(function (require, exports, module) { * Constants for the preferences defined in this file. */ var PREF_ENABLED = "enabled", - PREF_COLLAPSED = "collapsed"; + PREF_COLLAPSED = "collapsed", + PREF_ASYNC_TIMEOUT = "async_timeout"; var prefs = PreferencesManager.getExtensionPrefs("linting"); @@ -201,7 +202,7 @@ define(function (require, exports, module) { }); if (provider.scanFileAsync) { - setTimeout(function () { runPromise.resolve(null); }, 500); + setTimeout(function () { runPromise.resolve(null); }, prefs.get(PREF_ASYNC_TIMEOUT)); provider.scanFileAsync(fileText, file.fullPath) .then(function (scanResult) { PerfUtils.addMeasurement(perfTimerProvider); @@ -540,7 +541,7 @@ define(function (require, exports, module) { toggleCollapsed(prefs.get(PREF_COLLAPSED), true); }); - + prefs.definePreference(PREF_ASYNC_TIMEOUT, "number", 1000, "Number of milliseconds to wait for asynchronous code inspection provider results."); // Initialize items dependent on HTML DOM AppInit.htmlReady(function () { diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index 255655dc073..df8aa83ef2f 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -292,7 +292,7 @@ define(function (require, exports, module) { }); it("should timeout on a provider that takes too long", function () { - var provider = createAsyncCodeInspector("javascript async linter with sync impl", failLintResult(), 1000, true), + var provider = createAsyncCodeInspector("javascript async linter with sync impl", failLintResult(), 1500, true), result; CodeInspection.register("javascript", provider); @@ -302,7 +302,7 @@ define(function (require, exports, module) { result = r; }); - waitsForDone(promise, "file linting", 550); + waitsForDone(promise, "file linting", 5000); }); runs(function () { @@ -359,7 +359,7 @@ define(function (require, exports, module) { it("should return results for 3 providers when 2 completes and 1 times out", function () { var asyncProvider1 = createAsyncCodeInspector("javascript async linter 1", failLintResult(), 200, true), - asyncProvider2 = createAsyncCodeInspector("javascript async linter 2", failLintResult(), 1000, false), + asyncProvider2 = createAsyncCodeInspector("javascript async linter 2", failLintResult(), 1500, false), syncProvider3 = createCodeInspector("javascript sync linter 3", failLintResult()), result; CodeInspection.register("javascript", asyncProvider1); From a3291816d103da2d8d1ce47c05a9b884ac56ed55 Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Wed, 26 Mar 2014 21:17:50 -0300 Subject: [PATCH 11/17] Add test case for race condition. --- test/spec/CodeInspection-test.js | 40 +++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index df8aa83ef2f..adad4a745c4 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -290,7 +290,7 @@ define(function (require, exports, module) { }); }); - + it("should timeout on a provider that takes too long", function () { var provider = createAsyncCodeInspector("javascript async linter with sync impl", failLintResult(), 1500, true), result; @@ -418,6 +418,44 @@ define(function (require, exports, module) { }); }); + it("should show only warnings for the current file", function () { + CodeInspection.toggleEnabled(false); + + var firstTime = true, + deferred1 = new $.Deferred(), + deferred2 = new $.Deferred(); + + var asyncProvider = { + name: "Test Async Linter", + scanFileAsync: function () { + if (firstTime) { + firstTime = false; + return deferred1.promise(); + } else { + return deferred2.promise(); + } + } + }; + + CodeInspection.register("javascript", asyncProvider); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js", "errors.js"], "open test files")); + + runs(function () { + CodeInspection.toggleEnabled(true); + }); + // Close the file which was started to lint + runs(function () { + waitsForDone(CommandManager.execute(Commands.FILE_CLOSE), "timeout on FILE_CLOSE", 1000); + + // let the linter finish + deferred1.resolve(failLintResult()); + expect($("#problems-panel").is(":visible")).toBe(false); + deferred2.resolve(successfulLintResult()); + expect($("#problems-panel").is(":visible")).toBe(false); + }); + }); + it("should show problems panel after too many errors", function () { var lintResult = { errors: [ From 74358778a2e5fbc1e4d4cf6dc376b7a49208e12a Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Wed, 26 Mar 2014 22:28:16 -0300 Subject: [PATCH 12/17] Address nits and documentation review outcome. --- src/language/CodeInspection.js | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 72490a25bee..1fce036c5de 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -23,7 +23,7 @@ /*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4 */ -/*global define, $, Mustache, brackets, setTimeout */ +/*global define, $, Mustache, brackets */ /** * Manages linters and other code inspections on a per-language basis. Provides a UI and status indicator for @@ -77,7 +77,7 @@ define(function (require, exports, module) { */ var PREF_ENABLED = "enabled", PREF_COLLAPSED = "collapsed", - PREF_ASYNC_TIMEOUT = "async_timeout"; + PREF_ASYNC_TIMEOUT = "asyncTimeout"; var prefs = PreferencesManager.getExtensionPrefs("linting"); @@ -121,7 +121,7 @@ define(function (require, exports, module) { /** * @private - * @type {{languageId:string, Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:function(string, string):Object}>}} + * @type {{languageId:string, Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:?function(string, string):Object}>}} */ var _providers = {}; @@ -149,7 +149,7 @@ define(function (require, exports, module) { * Decision is made depending on the file extension. * * @param {!string} filePath - * @return ?{Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}>} provider + * @return ?{Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:?function(string, string):?{errors:!Array, aborted:boolean}}>} provider */ function getProvidersForPath(filePath) { return _providers[LanguageManager.getLanguageForPath(filePath).getId()]; @@ -161,20 +161,13 @@ define(function (require, exports, module) { * This method doesn't update the Brackets UI, just provides inspection results. * These results will reflect any unsaved changes present in the file if currently open. * - * If a provider implements scanFileAsync, then it will be used to scan the file. Otherwise, scanFile, - * a synchronous version of the function will be used. Provider must never reject a promise and resolve it - * with null in case the results cannot be retrieved for whatever reason. - * - * A code inspection provider's scanFileAsync must return a {$.Promise} object which must be resolved with - * ?{errors:!Array, aborted:boolean}}. - * * The Promise yields an array of provider-result pair objects (the result is the return value of the * provider's scanFile() - see register() for details). The result object may be null if there were no * errors from that provider. * If there are no providers registered for this file, the Promise yields null instead. * * @param {!File} file File that will be inspected for errors. - * @param {?Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}>} providerList + * @param {?Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:?function(string, string):?{errors:!Array, aborted:boolean}}>} providerList * @return {$.Promise} a jQuery promise that will be resolved with ?Array.<{provider:Object, result: ?{errors:!Array, aborted:boolean}}> */ function inspectFile(file, providerList) { @@ -197,12 +190,12 @@ define(function (require, exports, module) { var perfTimerProvider = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + file.fullPath), runPromise = new $.Deferred(); - runPromise.then(function (scanResult) { + runPromise.done(function (scanResult) { results.push({provider: provider, result: scanResult}); }); if (provider.scanFileAsync) { - setTimeout(function () { runPromise.resolve(null); }, prefs.get(PREF_ASYNC_TIMEOUT)); + window.setTimeout(function () { runPromise.resolve(null); }, prefs.get(PREF_ASYNC_TIMEOUT)); provider.scanFileAsync(fileText, file.fullPath) .then(function (scanResult) { PerfUtils.addMeasurement(perfTimerProvider); @@ -245,7 +238,7 @@ define(function (require, exports, module) { * change based on the number of problems reported and how many provider reported problems. * * @param {Number} numProblems - total number of problems across all providers - * @param {Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:function(string, string):Object}>} providersReportingProblems - providers that reported problems + * @param {Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:?function(string, string):Object}>} providersReportingProblems - providers that reported problems * @param {boolean} aborted - true if any provider returned a result with the 'aborted' flag set */ function updatePanelTitleAndStatusBar(numProblems, providersReportingProblems, aborted) { @@ -414,8 +407,12 @@ define(function (require, exports, module) { * code inspection, respectively, the asynchronous version will take precedence and will be used to * perform code inspection. * + * A code inspection provider's scanFileAsync must return a {$.Promise} object which must be + * resolved with ?{errors:!Array, aborted:boolean}}. Provider must never reject a promise and + * resolve it with null in case the results cannot be retrieved for whatever reason. + * * @param {string} languageId - * @param {{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}} provider + * @param {{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:?function(string, string):?{errors:!Array, aborted:boolean}}} provider * * Each error is: { pos:{line,ch}, endPos:?{line,ch}, message:string, type:?Type } * If type is unspecified, Type.WARNING is assumed. From fecb54327fd610744b71a0a5a626e010c48e930a Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Sat, 29 Mar 2014 23:58:38 -0300 Subject: [PATCH 13/17] Resolve with an error on timeout/failure. Add unit tests. Move strings to strings.js. --- src/language/CodeInspection.js | 32 ++++++++-- src/nls/root/strings.js | 6 ++ test/spec/CodeInspection-test.js | 106 ++++++++++++++++++++++++++++++- 3 files changed, 135 insertions(+), 9 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 1fce036c5de..fc6162fbf08 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -195,15 +195,28 @@ define(function (require, exports, module) { }); if (provider.scanFileAsync) { - window.setTimeout(function () { runPromise.resolve(null); }, prefs.get(PREF_ASYNC_TIMEOUT)); + window.setTimeout(function () { + // timeout error + var errTimeout = { + pos: { line: -1, col: 0}, + message: StringUtils.format(Strings.LINTER_TIMED_OUT, provider.name, prefs.get(PREF_ASYNC_TIMEOUT)), + type: Type.ERROR + }; + runPromise.resolve({errors: [errTimeout]}); + }, prefs.get(PREF_ASYNC_TIMEOUT)); provider.scanFileAsync(fileText, file.fullPath) - .then(function (scanResult) { + .done(function (scanResult) { PerfUtils.addMeasurement(perfTimerProvider); runPromise.resolve(scanResult); }) .fail(function (err) { + var errError = { + pos: {line: -1, col: 0}, + message: StringUtils.format(Strings.LINTER_FAILED, provider.name, err), + type: Type.ERROR + }; console.error("[CodeInspection] Provider " + provider.name + " (async) failed: " + err); - runPromise.resolve(null); + runPromise.resolve({errors: [errError]}); }); } else { try { @@ -211,8 +224,14 @@ define(function (require, exports, module) { PerfUtils.addMeasurement(perfTimerProvider); runPromise.resolve(scanResult); } catch (err) { + var errorMessage = (err.message) ? err.message : err; + var errError = { + pos: {line: -1, col: 0}, + message: StringUtils.format(Strings.LINTER_FAILED, provider.name, err), + type: Type.ERROR + }; console.error("[CodeInspection] Provider " + provider.name + " (sync) threw an error: " + err); - runPromise.resolve(null); + runPromise.resolve({errors: [errError]}); } } return runPromise.promise(); @@ -538,7 +557,7 @@ define(function (require, exports, module) { toggleCollapsed(prefs.get(PREF_COLLAPSED), true); }); - prefs.definePreference(PREF_ASYNC_TIMEOUT, "number", 1000, "Number of milliseconds to wait for asynchronous code inspection provider results."); + prefs.definePreference(PREF_ASYNC_TIMEOUT, "number", 1000, Strings.PREF_DESC_ASYNC_TIMEOUT); // Initialize items dependent on HTML DOM AppInit.htmlReady(function () { @@ -601,7 +620,8 @@ define(function (require, exports, module) { }); // Testing - exports._unregisterAll = _unregisterAll; + exports._unregisterAll = _unregisterAll; + exports._PREF_ASYNC_TIMEOUT = PREF_ASYNC_TIMEOUT; // Public API exports.register = register; diff --git a/src/nls/root/strings.js b/src/nls/root/strings.js index ef6d3213d29..6da6da4523d 100644 --- a/src/nls/root/strings.js +++ b/src/nls/root/strings.js @@ -179,6 +179,10 @@ define({ "FILE_FILTER_LIST_PREFIX" : "except", "FILE_FILTER_CLIPPED_SUFFIX" : "and {0} more", + /* + * Preference desriptions + */ + "PREF_DESC_ASYNC_TIMEOUT" : "Number of milliseconds to wait for asynchronous code inspection provider results.", /** * ProjectManager @@ -224,6 +228,8 @@ define({ "LINT_DISABLED" : "Linting is disabled", "NO_LINT_AVAILABLE" : "No linter available for {0}", "NOTHING_TO_LINT" : "Nothing to lint", + "LINTER_TIMED_OUT" : "{0} has timed out after waiting for {1} ms", + "LINTER_FAILED" : "{0} terminated with error: {1}", /** diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index adad4a745c4..4035fdb4f50 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -22,7 +22,7 @@ */ /*jslint vars: true, plusplus: true, devel: true, browser: true, nomen: true, indent: 4, maxerr: 50 */ -/*global define, describe, it, expect, beforeEach, beforeFirst, afterEach, afterLast, waitsFor, runs, brackets, waitsForDone, spyOn, xit, jasmine */ +/*global define, describe, it, expect, beforeEach, beforeFirst, afterEach, afterLast, waitsFor, waits, runs, brackets, waitsForDone, spyOn, xit, jasmine */ define(function (require, exports, module) { "use strict"; @@ -42,7 +42,8 @@ define(function (require, exports, module) { CodeInspection, CommandManager, Commands = require("command/Commands"), - EditorManager; + EditorManager, + prefs; var toggleJSLintResults = function (visible) { $("#status-inspection").triggerHandler("click"); @@ -111,6 +112,7 @@ define(function (require, exports, module) { Strings = testWindow.require("strings"); CommandManager = brackets.test.CommandManager; EditorManager = brackets.test.EditorManager; + prefs = brackets.test.PreferencesManager.getExtensionPrefs("linting"); CodeInspection = brackets.test.CodeInspection; CodeInspection.toggleEnabled(true); }); @@ -388,7 +390,9 @@ define(function (require, exports, module) { break; case asyncProvider2.name: expect(result[i].result).toBeDefined(); - expect(result[i].result).toBeNull(); + expect(result[i].result.errors.length).toBe(1); + expect(result[i].result.errors[0].pos).toEqual({line: -1, col: 0}); + expect(result[i].result.errors[0].message).toBe(StringUtils.format(Strings.LINTER_TIMED_OUT, "javascript async linter 2", prefs.get(CodeInspection._PREF_ASYNC_TIMEOUT))); break; default: expect(true).toBe(false); @@ -761,6 +765,102 @@ define(function (require, exports, module) { expect(tooltip).toBe(StringUtils.format(Strings.ERRORS_PANEL_TITLE_MULTIPLE, 2)); }); }); + + it("should report an async linter which has timeout", function () { + var codeInspectorToTimeout = createAsyncCodeInspector("SlowAsyncLinter", { + errors: [ + { + pos: { line: 1, ch: 0 }, + message: "SlowAsyncLinter was here", + type: CodeInspection.Type.WARNING + }, + { + pos: { line: 2, ch: 0 }, + message: "SlowAsyncLinter was here as well", + type: CodeInspection.Type.WARNING + } + ] + }, 4000, false); + + CodeInspection.register("javascript", codeInspectorToTimeout); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); + + waits(prefs.get(CodeInspection._PREF_ASYNC_TIMEOUT) + 10); + + runs(function () { + var $problemsPanel = $("#problems-panel"); + expect($problemsPanel.is(":visible")).toBe(true); + + var $problemsPanelTitle = $("#problems-panel .title").text(); + expect($problemsPanelTitle).toBe(StringUtils.format(Strings.SINGLE_ERROR, "SlowAsyncLinter")); + + var $problemsReported = $("#problems-panel .bottom-panel-table .line-text"); + expect($problemsReported.length).toBe(1); + expect($problemsReported.text()) + .toBe( + StringUtils.format(Strings.LINTER_TIMED_OUT, "SlowAsyncLinter", prefs.get(CodeInspection._PREF_ASYNC_TIMEOUT)) + ); + }); + }); + + it("should report a buggy async linter", function () { + var errorMessage = "I'm full of bugs on purpose", + providerName = "Buggy Async Linter", + buggyAsyncProvider = { + name: providerName, + scanFileAsync: function () { + var deferred = new $.Deferred(); + deferred.reject(errorMessage); + return deferred.promise(); + } + }; + + CodeInspection.register("javascript", buggyAsyncProvider); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); + + runs(function () { + var $problemsPanel = $("#problems-panel"); + expect($problemsPanel.is(":visible")).toBe(true); + + var $problemsPanelTitle = $("#problems-panel .title").text(); + expect($problemsPanelTitle).toBe(StringUtils.format(Strings.SINGLE_ERROR, providerName)); + + var $problemsReported = $("#problems-panel .bottom-panel-table .line-text"); + expect($problemsReported.length).toBe(1); + expect($problemsReported.text()) + .toBe(StringUtils.format(Strings.LINTER_FAILED, providerName, errorMessage)); + }); + }); + + it("should report a buggy sync linter", function () { + var errorMessage = "I'm synchronous, but still full of bugs", + providerName = "Buggy Sync Linter", + buggySyncProvider = { + name: providerName, + scanFile: function () { + throw new Error(errorMessage); + } + }; + + CodeInspection.register("javascript", buggySyncProvider); + + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); + + runs(function () { + var $problemsPanel = $("#problems-panel"); + expect($problemsPanel.is(":visible")).toBe(true); + + var $problemsPanelTitle = $("#problems-panel .title").text(); + expect($problemsPanelTitle).toBe(StringUtils.format(Strings.SINGLE_ERROR, providerName)); + + var $problemsReported = $("#problems-panel .bottom-panel-table .line-text"); + expect($problemsReported.length).toBe(1); + expect($problemsReported.text()) + .toBe(StringUtils.format(Strings.LINTER_FAILED, providerName, new Error(errorMessage))); + }); + }); }); describe("Code Inspector Registration", function () { From 6f5e030beaaba9dfe83c1d3aa88f6cf9961611f3 Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Sun, 30 Mar 2014 23:29:06 -0300 Subject: [PATCH 14/17] Keep the order of the providers when reporting. Currently the order is defined by the order of registration. --- src/language/CodeInspection.js | 8 +++++++- test/spec/CodeInspection-test.js | 25 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index fc6162fbf08..8ec7f161b79 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -324,7 +324,13 @@ define(function (require, exports, module) { if (currentDoc !== DocumentManager.getCurrentDocument()) { return; } - + + // sync async may have pushed results in different order, restore the original order + results.sort(function (a, b) { + // actual provider list may have changed in the process, but we don't care + return _.indexOf(providerList, a.provider) - _.indexOf(providerList, b.provider); + }); + // how many errors in total? var errors = results.reduce(function (a, item) { return a + (item.result ? item.result.errors.length : 0); }, 0); diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index 4035fdb4f50..d35e77918e4 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -861,6 +861,31 @@ define(function (require, exports, module) { .toBe(StringUtils.format(Strings.LINTER_FAILED, providerName, new Error(errorMessage))); }); }); + + it("should keep the order as per registration", function () { + var asyncProvider1 = createAsyncCodeInspector("javascript async linter 1", failLintResult(), 400, true), + asyncProvider2 = createAsyncCodeInspector("javascript async linter 2", failLintResult(), 300, false), + syncProvider3 = createCodeInspector("javascript sync linter 3", failLintResult()), + registrationOrder = [asyncProvider1, asyncProvider2, syncProvider3], + i, + expected = ""; + + for (i = 0; i < registrationOrder.length; i++) { + CodeInspection.register("javascript", registrationOrder[i]); + expected += registrationOrder[i].name + " " + "(1) "; + } + + waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); + + waits(410); + + runs(function () { + expect($("#problems-panel .inspector-section").text().trim().replace(/\s+/g, " ")) + // actual string expected: + //.toBe("javascript async linter 1 (1) javascript async linter 2 (1) javascript sync linter 3 (1)"); + .toBe(expected.trim()); + }); + }); }); describe("Code Inspector Registration", function () { From c786e697955f7d7da00be1a62b50e68ce112ae2a Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Mon, 31 Mar 2014 20:38:23 -0300 Subject: [PATCH 15/17] Check for current promise instead of document. Keep track of the last promise which inspectFile returns to prevent any stale promises from updating the UI. --- src/language/CodeInspection.js | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 8ec7f161b79..efaa3097b68 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -130,6 +130,15 @@ define(function (require, exports, module) { * @type {boolean} */ var _hasErrors; + + /** + * Promise of the returned by the last call to inspectFile or null if linting is disabled. Used to prevent any stale promises + * to cause updates of the UI. + * + * @private + * @type {$.Promise} + */ + var _currentPromise = null; /** * Enable or disable the "Go to First Error" command @@ -302,6 +311,7 @@ define(function (require, exports, module) { function run() { if (!_enabled) { _hasErrors = false; + _currentPromise = null; Resizer.hide($problemsPanel); StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-disabled", Strings.LINT_DISABLED); setGotoEnabled(false); @@ -319,9 +329,9 @@ define(function (require, exports, module) { var providersReportingProblems = []; // run all the providers registered for this file type - inspectFile(currentDoc.file, providerList).then(function (results) { - // check if current document wasn't changed while inspectFile was running - if (currentDoc !== DocumentManager.getCurrentDocument()) { + (_currentPromise = inspectFile(currentDoc.file, providerList)).then(function (results) { + // check if promise has not changed while inspectFile was running + if (this !== _currentPromise) { return; } @@ -408,6 +418,7 @@ define(function (require, exports, module) { } else { // No provider for current file _hasErrors = false; + _currentPromise = null; Resizer.hide($problemsPanel); var language = currentDoc && LanguageManager.getLanguageForPath(currentDoc.file.fullPath); if (language) { From a075ab4d7cf83fa39d3f9873cf64e9dbad38232a Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Wed, 2 Apr 2014 19:26:18 -0300 Subject: [PATCH 16/17] Remove left-over documentation sentence. --- src/language/CodeInspection.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index efaa3097b68..3d20f8c78c8 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -443,9 +443,8 @@ define(function (require, exports, module) { * code inspection, respectively, the asynchronous version will take precedence and will be used to * perform code inspection. * - * A code inspection provider's scanFileAsync must return a {$.Promise} object which must be - * resolved with ?{errors:!Array, aborted:boolean}}. Provider must never reject a promise and - * resolve it with null in case the results cannot be retrieved for whatever reason. + * A code inspection provider's scanFileAsync must return a {$.Promise} object which should be + * resolved with ?{errors:!Array, aborted:boolean}}. * * @param {string} languageId * @param {{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:?function(string, string):?{errors:!Array, aborted:boolean}}} provider From a46b9df1672f0a7da7dedcc3c4e168b32220af89 Mon Sep 17 00:00:00 2001 From: Arzhan Kinzhalin Date: Fri, 4 Apr 2014 18:53:45 -0300 Subject: [PATCH 17/17] Address review comments. - Sort the results immediately in inspectFile. - Refactor the tests. Set timeout low for the tests to run faster. - Increase timeout to 10 seconds. --- src/language/CodeInspection.js | 19 ++++++++----------- test/spec/CodeInspection-test.js | 28 +++++++++++++++++----------- 2 files changed, 25 insertions(+), 22 deletions(-) diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index 3d20f8c78c8..5d6460d21ce 100644 --- a/src/language/CodeInspection.js +++ b/src/language/CodeInspection.js @@ -233,7 +233,6 @@ define(function (require, exports, module) { PerfUtils.addMeasurement(perfTimerProvider); runPromise.resolve(scanResult); } catch (err) { - var errorMessage = (err.message) ? err.message : err; var errError = { pos: {line: -1, col: 0}, message: StringUtils.format(Strings.LINTER_FAILED, provider.name, err), @@ -248,6 +247,10 @@ define(function (require, exports, module) { }, false); masterPromise.then(function () { + // sync async may have pushed results in different order, restore the original order + results.sort(function (a, b) { + return providerList.indexOf(a.provider) - providerList.indexOf(b.provider); + }); PerfUtils.addMeasurement(perfTimerInspector); response.resolve(results); }); @@ -335,12 +338,6 @@ define(function (require, exports, module) { return; } - // sync async may have pushed results in different order, restore the original order - results.sort(function (a, b) { - // actual provider list may have changed in the process, but we don't care - return _.indexOf(providerList, a.provider) - _.indexOf(providerList, b.provider); - }); - // how many errors in total? var errors = results.reduce(function (a, item) { return a + (item.result ? item.result.errors.length : 0); }, 0); @@ -573,7 +570,7 @@ define(function (require, exports, module) { toggleCollapsed(prefs.get(PREF_COLLAPSED), true); }); - prefs.definePreference(PREF_ASYNC_TIMEOUT, "number", 1000, Strings.PREF_DESC_ASYNC_TIMEOUT); + prefs.definePreference(PREF_ASYNC_TIMEOUT, "number", 10000); // Initialize items dependent on HTML DOM AppInit.htmlReady(function () { @@ -636,13 +633,13 @@ define(function (require, exports, module) { }); // Testing - exports._unregisterAll = _unregisterAll; - exports._PREF_ASYNC_TIMEOUT = PREF_ASYNC_TIMEOUT; + exports._unregisterAll = _unregisterAll; + exports._PREF_ASYNC_TIMEOUT = PREF_ASYNC_TIMEOUT; // Public API exports.register = register; exports.Type = Type; exports.toggleEnabled = toggleEnabled; exports.inspectFile = inspectFile; - exports.requestRun = run; + exports.requestRun = run; }); diff --git a/test/spec/CodeInspection-test.js b/test/spec/CodeInspection-test.js index d35e77918e4..05e9cf8bd18 100644 --- a/test/spec/CodeInspection-test.js +++ b/test/spec/CodeInspection-test.js @@ -122,6 +122,11 @@ define(function (require, exports, module) { SpecRunnerUtils.loadProjectInTestWindow(testFolder); }); }); + + beforeEach(function () { + // this is to make the tests run faster + prefs.set(CodeInspection._PREF_ASYNC_TIMEOUT, 500); + }); afterEach(function () { testWindow.closeAllFiles(); @@ -360,9 +365,10 @@ define(function (require, exports, module) { }); it("should return results for 3 providers when 2 completes and 1 times out", function () { - var asyncProvider1 = createAsyncCodeInspector("javascript async linter 1", failLintResult(), 200, true), - asyncProvider2 = createAsyncCodeInspector("javascript async linter 2", failLintResult(), 1500, false), - syncProvider3 = createCodeInspector("javascript sync linter 3", failLintResult()), + var timeout = prefs.get(CodeInspection._PREF_ASYNC_TIMEOUT), + asyncProvider1 = createAsyncCodeInspector("javascript async linter 1", failLintResult(), 200, true), + asyncProvider2 = createAsyncCodeInspector("javascript async linter 2", failLintResult(), timeout + 10, false), + syncProvider3 = createCodeInspector("javascript sync linter 3", failLintResult()), result; CodeInspection.register("javascript", asyncProvider1); CodeInspection.register("javascript", asyncProvider2); @@ -374,7 +380,7 @@ define(function (require, exports, module) { result = r; }); - waitsForDone(promise, "file linting", 5000); + waitsForDone(promise, "file linting", timeout + 10); }); runs(function () { @@ -447,11 +453,11 @@ define(function (require, exports, module) { runs(function () { CodeInspection.toggleEnabled(true); + CommandManager.execute(Commands.FILE_CLOSE); }); + // Close the file which was started to lint runs(function () { - waitsForDone(CommandManager.execute(Commands.FILE_CLOSE), "timeout on FILE_CLOSE", 1000); - // let the linter finish deferred1.resolve(failLintResult()); expect($("#problems-panel").is(":visible")).toBe(false); @@ -766,7 +772,7 @@ define(function (require, exports, module) { }); }); - it("should report an async linter which has timeout", function () { + it("should report an async linter which has timed out", function () { var codeInspectorToTimeout = createAsyncCodeInspector("SlowAsyncLinter", { errors: [ { @@ -780,13 +786,13 @@ define(function (require, exports, module) { type: CodeInspection.Type.WARNING } ] - }, 4000, false); + }, prefs.get(CodeInspection._PREF_ASYNC_TIMEOUT) + 10, false); CodeInspection.register("javascript", codeInspectorToTimeout); waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file"); - waits(prefs.get(CodeInspection._PREF_ASYNC_TIMEOUT) + 10); + waits(prefs.get(CodeInspection._PREF_ASYNC_TIMEOUT) + 20); runs(function () { var $problemsPanel = $("#problems-panel"); @@ -804,7 +810,7 @@ define(function (require, exports, module) { }); }); - it("should report a buggy async linter", function () { + it("should report an async linter which throws an exception", function () { var errorMessage = "I'm full of bugs on purpose", providerName = "Buggy Async Linter", buggyAsyncProvider = { @@ -834,7 +840,7 @@ define(function (require, exports, module) { }); }); - it("should report a buggy sync linter", function () { + it("should report a sync linter which throws an exception", function () { var errorMessage = "I'm synchronous, but still full of bugs", providerName = "Buggy Sync Linter", buggySyncProvider = {