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

Code Inspection: minor fixes & more unit tests #7650

Merged
merged 1 commit into from
May 9, 2014
Merged

Code Inspection: minor fixes & more unit tests #7650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions src/language/CodeInspection.js
View file
Open in desktop
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any chance inspectionResult.result.errors could be undefined/null?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I see. The doc string specifically states that errors must be non-null. OK.

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
Open in desktop
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
Open in desktop
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