Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Commit

Permalink
Code Inspection: minor fixes & more unit tests:
Browse files Browse the repository at this point in the history 
- Fix bug: if one linter returns errors and another returns errors:[], the
no-errors linter is shown in the panel as an empty heading. (Bug doesn't
happen if it returned null instead; but both are valid ways of indicating
zero errors).
- Improve docs on register() - more concise, more detail on how to indicate
zero errors
- Add unit tests for the bug fixed above plus several more async linting
cases that were fixed late in PR #6530
- Fix mislabeled existing unit test for reject()ing async linters
- Fix stray whitespace that snuck into merge c3411bb
  • Loading branch information
@peterflynn
peterflynn committed Apr 25, 2014
1 parent 4d968b2 commit 168b174
Show file tree
Hide file tree
Showing 3 changed files with 187 additions and 45 deletions.
18 changes: 9 additions & 9 deletions src/language/CodeInspection.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ define(function (require, exports, module) {
aborted = true;
}

if (inspectionResult.result.errors) {
if (inspectionResult.result.errors.length) {
allErrors.push({
providerName: provider.name,
results: inspectionResult.result.errors
Expand Down Expand Up @@ -436,18 +436,18 @@ define(function (require, exports, module) {
* 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}}.
*
* Providers implement scanFile() if results are available synchronously, or scanFileAsync() if results
* may require an async wait (if both are implemented, scanFile() is ignored). scanFileAsync() returns
* a {$.Promise} object resolved with the same type of value as scanFile() is expected to return.
* Rejecting the promise is treated as an internal error in the provider.
*
* @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.
* If no errors found, return either null or an object with a zero-length `errors` array.
*/
function register(languageId, provider) {
if (!_providers[languageId]) {
Expand Down
1 change: 0 additions & 1 deletion src/nls/root/strings.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,6 @@ define({
"FILE_FILTER_INSTRUCTIONS" : "Exclude files and folders matching any of the following strings / substrings or <a href='{0}' title='{0}'>wildcards</a>. 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}",
Expand Down
213 changes: 178 additions & 35 deletions test/spec/CodeInspection-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ define(function (require, exports, module) {
CodeInspection,
CommandManager,
Commands = require("command/Commands"),
DocumentManager,
EditorManager,
prefs;

Expand Down Expand Up @@ -111,6 +112,7 @@ define(function (require, exports, module) {
brackets = testWindow.brackets;
Strings = testWindow.require("strings");
CommandManager = brackets.test.CommandManager;
DocumentManager = brackets.test.DocumentManager;
EditorManager = brackets.test.EditorManager;
prefs = brackets.test.PreferencesManager.getExtensionPrefs("linting");
CodeInspection = brackets.test.CodeInspection;
Expand All @@ -137,6 +139,7 @@ define(function (require, exports, module) {
$ = null;
brackets = null;
CommandManager = null;
DocumentManager = null;
EditorManager = null;
SpecRunnerUtils.closeTestWindow();
});
Expand Down Expand Up @@ -413,8 +416,29 @@ define(function (require, exports, module) {
describe("Code Inspection UI", function () {
beforeEach(function () {
CodeInspection._unregisterAll();
CodeInspection.toggleEnabled(true);
});

// Utility to create an async provider where the testcase can control when each async result resolves
function makeAsyncLinter() {
return {
name: "Test Async Linter",
scanFileAsync: function (text, fullPath) {
if (!this.futures[fullPath]) {
this.futures[fullPath] = [];
this.filesCalledOn.push(fullPath);
}

var result = new $.Deferred();
this.futures[fullPath].push(result);
return result.promise();
},
futures: {}, // map from full path to array of Deferreds (in call order)
filesCalledOn: [] // in order of first call for each path
};
}


it("should run test linter when a JavaScript document opens and indicate errors in the panel", function () {
var codeInspector = createCodeInspector("javascript linter", failLintResult());
CodeInspection.register("javascript", codeInspector);
Expand All @@ -427,45 +451,124 @@ define(function (require, exports, module) {
expect($statusBar.is(":visible")).toBe(true);
});
});

it("should show only warnings for the current file", function () {
it("should ignore async results from previous 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();
}
}
};

var asyncProvider = makeAsyncLinter();
CodeInspection.register("javascript", asyncProvider);

waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js", "errors.js"], "open test files"));
waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js", "errors.js"]), "open test files");

var errorsJS = SpecRunnerUtils.makeAbsolute("errors.js"),
noErrorsJS = SpecRunnerUtils.makeAbsolute("no-errors.js");

runs(function () {
// Start linting the first file
CodeInspection.toggleEnabled(true);
CommandManager.execute(Commands.FILE_CLOSE);
expect(asyncProvider.filesCalledOn).toEqual([errorsJS]);

// Close that file, switching to the 2nd one
waitsForDone(CommandManager.execute(Commands.FILE_CLOSE));
});

// Close the file which was started to lint
runs(function () {
// let the linter finish
deferred1.resolve(failLintResult());
// Verify that we started linting the 2nd file
expect(DocumentManager.getCurrentDocument().file.fullPath).toBe(noErrorsJS);
expect(asyncProvider.filesCalledOn).toEqual([errorsJS, noErrorsJS]);

// Finish old (stale) linting session - verify results not shown
asyncProvider.futures[errorsJS][0].resolve(failLintResult());
expect($("#problems-panel").is(":visible")).toBe(false);
deferred2.resolve(successfulLintResult());

// Finish new (current) linting session
asyncProvider.futures[noErrorsJS][0].resolve(successfulLintResult());
expect($("#problems-panel").is(":visible")).toBe(false);
});
});

it("should ignore async results from previous run in same file - finishing in order", function () {
CodeInspection.toggleEnabled(false);

var asyncProvider = makeAsyncLinter();
CodeInspection.register("javascript", asyncProvider);

waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test files");

var noErrorsJS = SpecRunnerUtils.makeAbsolute("no-errors.js");

runs(function () {
// Start linting the file
CodeInspection.toggleEnabled(true);
expect(asyncProvider.filesCalledOn).toEqual([noErrorsJS]);

// "Modify" the file
$(DocumentManager).triggerHandler("documentSaved", DocumentManager.getCurrentDocument());
expect(asyncProvider.futures[noErrorsJS].length).toBe(2);

// Finish old (stale) linting session - verify results not shown
asyncProvider.futures[noErrorsJS][0].resolve(failLintResult());
expect($("#problems-panel").is(":visible")).toBe(false);

// Finish new (current) linting session - verify results are shown
asyncProvider.futures[noErrorsJS][1].resolve(failLintResult());
expect($("#problems-panel").is(":visible")).toBe(true);
});
});

it("should ignore async results from previous run in same file - finishing reverse order", function () {
CodeInspection.toggleEnabled(false);

var asyncProvider = makeAsyncLinter();
CodeInspection.register("javascript", asyncProvider);

waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test files");

var noErrorsJS = SpecRunnerUtils.makeAbsolute("no-errors.js");

runs(function () {
// Start linting the file
CodeInspection.toggleEnabled(true);
expect(asyncProvider.filesCalledOn).toEqual([noErrorsJS]);

// "Modify" the file
$(DocumentManager).triggerHandler("documentSaved", DocumentManager.getCurrentDocument());
expect(asyncProvider.futures[noErrorsJS].length).toBe(2);

// Finish new (current) linting session - verify results are shown
asyncProvider.futures[noErrorsJS][1].resolve(failLintResult());
expect($("#problems-panel").is(":visible")).toBe(true);

// Finish old (stale) linting session - verify results don't replace current results
asyncProvider.futures[noErrorsJS][0].resolve(successfulLintResult());
expect($("#problems-panel").is(":visible")).toBe(true);
});
});

it("should ignore async results after linting disabled", function () {
CodeInspection.toggleEnabled(false);

var asyncProvider = makeAsyncLinter();
CodeInspection.register("javascript", asyncProvider);

waitsForDone(SpecRunnerUtils.openProjectFiles(["no-errors.js"]), "open test files");

var noErrorsJS = SpecRunnerUtils.makeAbsolute("no-errors.js");

runs(function () {
// Start linting the file
CodeInspection.toggleEnabled(true);
expect(asyncProvider.filesCalledOn).toEqual([noErrorsJS]);

// Disable linting
CodeInspection.toggleEnabled(false);

// Finish old (stale) linting session - verify results not shown
asyncProvider.futures[noErrorsJS][0].resolve(failLintResult());
expect($("#problems-panel").is(":visible")).toBe(false);
});
});

it("should show problems panel after too many errors", function () {
var lintResult = {
errors: [
Expand Down Expand Up @@ -517,8 +620,8 @@ define(function (require, exports, module) {
});
});

it("should not show the problems panel when there is no linting error", function () {
var codeInspector = createCodeInspector("javascript linter", successfulLintResult());
it("should not show the problems panel when there is no linting error - empty errors array", function () {
var codeInspector = createCodeInspector("javascript linter", {errors: []});
CodeInspection.register("javascript", codeInspector);

waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000);
Expand All @@ -530,19 +633,20 @@ define(function (require, exports, module) {
});
});

it("status icon should toggle Errors panel when errors present", function () {
var codeInspector = createCodeInspector("javascript linter", failLintResult());
it("should not show the problems panel when there is no linting error - null result", function () {
var codeInspector = createCodeInspector("javascript linter", null);
CodeInspection.register("javascript", codeInspector);

waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file");
waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000);

runs(function () {
toggleJSLintResults(false);
toggleJSLintResults(true);
expect($("#problems-panel").is(":visible")).toBe(false);
var $statusBar = $("#status-inspection");
expect($statusBar.is(":visible")).toBe(true);
});
});

it("should run two linters and display two expanded collapsible sections in the errors panel", function () {
it("should display two expanded, collapsible sections in the errors panel when two linters have errors", function () {
var codeInspector1 = createCodeInspector("javascript linter 1", failLintResult());
var codeInspector2 = createCodeInspector("javascript linter 2", failLintResult());
CodeInspection.register("javascript", codeInspector1);
Expand All @@ -561,17 +665,56 @@ define(function (require, exports, module) {
});
});

it("should run the linter and display no collapsible header section in the errors panel", function () {
var codeInspector1 = createCodeInspector("javascript linter 1", failLintResult());
it("should display no header section when only one linter has errors", function () {
var codeInspector1 = createCodeInspector("javascript linter 1", failLintResult()),
codeInspector2 = createCodeInspector("javascript linter 2", {errors: []}), // 1st way of reporting 0 errors
codeInspector3 = createCodeInspector("javascript linter 3", null); // 2nd way of reporting 0 errors
CodeInspection.register("javascript", codeInspector1);
CodeInspection.register("javascript", codeInspector2);
CodeInspection.register("javascript", codeInspector3);

waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000);

runs(function () {
expect($("#problems-panel").is(":visible")).toBe(true);
expect($(".inspector-section").is(":visible")).toBeFalsy();
});
});

it("should only display header sections for linters with errors", function () {
var codeInspector1 = createCodeInspector("javascript linter 1", failLintResult()),
codeInspector2 = createCodeInspector("javascript linter 2", {errors: []}), // 1st way of reporting 0 errors
codeInspector3 = createCodeInspector("javascript linter 3", null), // 2nd way of reporting 0 errors
codeInspector4 = createCodeInspector("javascript linter 4", failLintResult());
CodeInspection.register("javascript", codeInspector1);
CodeInspection.register("javascript", codeInspector2);
CodeInspection.register("javascript", codeInspector3);
CodeInspection.register("javascript", codeInspector4);

waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file", 5000);

runs(function () {
expect($("#problems-panel").is(":visible")).toBe(true);

var $inspectorSections = $(".inspector-section td");
expect($inspectorSections.length).toEqual(2);
expect($inspectorSections[0].innerHTML.indexOf("javascript linter 1 (1)")).not.toBe(-1);
expect($inspectorSections[1].innerHTML.indexOf("javascript linter 4 (1)")).not.toBe(-1);
});
});

it("status icon should toggle Errors panel when errors present", function () {
var codeInspector = createCodeInspector("javascript linter", failLintResult());
CodeInspection.register("javascript", codeInspector);

waitsForDone(SpecRunnerUtils.openProjectFiles(["errors.js"]), "open test file");

runs(function () {
toggleJSLintResults(false);
toggleJSLintResults(true);
});
});

it("status icon should not toggle Errors panel when no errors present", function () {
var codeInspector = createCodeInspector("javascript linter", successfulLintResult());
CodeInspection.register("javascript", codeInspector);
Expand Down Expand Up @@ -810,7 +953,7 @@ define(function (require, exports, module) {
});
});

it("should report an async linter which throws an exception", function () {
it("should report an async linter which rejects", function () {
var errorMessage = "I'm full of bugs on purpose",
providerName = "Buggy Async Linter",
buggyAsyncProvider = {
Expand Down

0 comments on commit 168b174

Please sign in to comment.