diff --git a/src/language/CodeInspection.js b/src/language/CodeInspection.js index ae3a55d8bbf..5d6460d21ce 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 = "asyncTimeout"; var prefs = PreferencesManager.getExtensionPrefs("linting"); @@ -120,7 +121,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 = {}; @@ -129,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 @@ -148,7 +158,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()]; @@ -166,7 +176,7 @@ define(function (require, exports, module) { * 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) { @@ -179,29 +189,72 @@ 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); + 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(); + + runPromise.done(function (scanResult) { results.push({provider: provider, result: scanResult}); - } catch (err) { - console.error("[CodeInspection] Provider " + provider.name + " threw an error: " + err); - response.reject(err); - return; + }); + + if (provider.scanFileAsync) { + 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) + .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({errors: [errError]}); + }); + } else { + try { + var scanResult = provider.scanFile(fileText, file.fullPath); + PerfUtils.addMeasurement(perfTimerProvider); + runPromise.resolve(scanResult); + } catch (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({errors: [errError]}); + } } + return runPromise.promise(); - PerfUtils.addMeasurement(perfTimerProvider); + }, 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); }); - PerfUtils.addMeasurement(perfTimerInspector); - - response.resolve(results); }) .fail(function (err) { console.error("[CodeInspection] Could not read file for inspection: " + file.fullPath); @@ -216,7 +269,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) { @@ -261,6 +314,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); @@ -278,12 +332,12 @@ 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; } - + // how many errors in total? var errors = results.reduce(function (a, item) { return a + (item.result ? item.result.errors.length : 0); }, 0); @@ -361,6 +415,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) { @@ -380,9 +435,16 @@ 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. + * + * 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, 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. @@ -391,7 +453,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 @@ -399,7 +461,7 @@ define(function (require, exports, module) { return registeredProvider.name === "JSLint"; }); } - + _providers[languageId].push(provider); run(); // in case a file of this type is open currently @@ -508,7 +570,7 @@ define(function (require, exports, module) { toggleCollapsed(prefs.get(PREF_COLLAPSED), true); }); - + prefs.definePreference(PREF_ASYNC_TIMEOUT, "number", 10000); // Initialize items dependent on HTML DOM AppInit.htmlReady(function () { @@ -571,12 +633,13 @@ define(function (require, exports, module) { }); // Testing - exports._unregisterAll = _unregisterAll; + 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/src/nls/root/strings.js b/src/nls/root/strings.js index 1856cdec144..538a9d62735 100644 --- a/src/nls/root/strings.js +++ b/src/nls/root/strings.js @@ -179,6 +179,7 @@ define({ "FILE_FILTER_INSTRUCTIONS" : "Exclude files and folders matching any of the following strings / substrings or wildcards. Enter each string on a new line.", "FILE_FILTER_LIST_PREFIX" : "except", "FILE_FILTER_CLIPPED_SUFFIX" : "and {0} more", + "FILTER_COUNTING_FILES" : "Counting files\u2026", "FILTER_FILE_COUNT" : "Allows {0} of {1} files {2}", "FILTER_FILE_COUNT_ALL" : "Allows all {0} files {1}", @@ -239,6 +240,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 0068a84d622..05e9cf8bd18 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"); @@ -61,6 +62,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) { + provider.scanFile = function () { + return result; + }; + spyOn(provider, "scanFile").andCallThrough(); + } + + return provider; + } function successfulLintResult() { return {errors: []}; @@ -88,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); }); @@ -97,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(); @@ -250,6 +280,134 @@ 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(), 1500, true), + result; + CodeInspection.register("javascript", provider); + + runs(function () { + var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); + promise.done(function (r) { + result = r; + }); + + waitsForDone(promise, "file linting", 5000); + }); + + 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 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); + CodeInspection.register("javascript", syncProvider3); + + runs(function () { + var promise = CodeInspection.inspectFile(simpleJavascriptFileEntry); + promise.done(function (r) { + result = r; + }); + + waitsForDone(promise, "file linting", timeout + 10); + }); + + 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.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); + break; + } + } + }); + }); + }); describe("Code Inspection UI", function () { @@ -270,6 +428,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); + CommandManager.execute(Commands.FILE_CLOSE); + }); + + // Close the file which was started to lint + runs(function () { + // 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: [ @@ -575,6 +771,127 @@ define(function (require, exports, module) { expect(tooltip).toBe(StringUtils.format(Strings.ERRORS_PANEL_TITLE_MULTIPLE, 2)); }); }); + + it("should report an async linter which has timed out", 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 + } + ] + }, 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) + 20); + + 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 an async linter which throws an exception", 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 sync linter which throws an exception", 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))); + }); + }); + + 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 () {