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

Commit 5dc6489

Browse files
peterflynnpeterflynn
committed
Cleanups, mainly to unit tests:
- Improve docs in SpecRunnerUtils - Add assertion in SRU in case a mock Document is created without an Editor but then used in a way that would auto-create an Editor (which won't get cleaned up) - Add assertion in _destroyEditorIfUnneeded() since its name makes it easy to call with the wrong type (including via SRU.destroyMockEditor()) - Rename HTML code hints suite & remove unused var - Minor docs cleanups in LanguageManager (incorrect usage of "i.e.", missing docs for new fileNames option, etc.) - Add unit tests for install dialog cases where installer comes back with a result after clicking Cancel but before Cancel times out, or comes back vary late after timeout once user has opted to close the dialog - Remove a duplicated LanguageManager test (due to f67cfcd) - Add a few more StringMatch tests to ensure tricky cases don't break in the opposite direction as before
1 parent 60dc63f commit 5dc6489

File tree

7 files changed

+144
-59
lines changed

7 files changed

+144
-59
lines changed

‎src/editor/EditorManager.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,9 @@ define(function (require, exports, module) {
295295
var editor = document._masterEditor;
296296

297297
if (!editor) {
298+
if (!(document instanceof DocumentManager.Document)) {
299+
throw new Error("_destroyEditorIfUnneeded() takes a Document arg, not an Editor");
300+
}
298301
return;
299302
}
300303

‎src/extensions/default/HTMLCodeHints/unittests.js

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ define(function (require, exports, module) {
3434
CodeHintManager = brackets.getModule("editor/CodeHintManager"),
3535
HTMLCodeHints = require("main");
3636

37-
describe("HTML Attribute Hinting", function () {
37+
describe("HTML Code Hinting", function () {
3838

3939
var defaultContent = "<!doctype html>\n" +
4040
"<html>\n" +
@@ -49,7 +49,6 @@ define(function (require, exports, module) {
4949
"</body>\n" +
5050
"</html>\n";
5151

52-
var testWindow;
5352
var testDocument, testEditor;
5453

5554
beforeEach(function () {
@@ -682,5 +681,5 @@ define(function (require, exports, module) {
682681
});
683682

684683

685-
}); // describe("HTML Attribute Hinting"
684+
}); // describe("HTML Code Hinting"
686685
});

‎src/language/LanguageManager.js

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ define(function (require, exports, module) {
173173
/**
174174
* Resolves a language ID to a Language object.
175175
* File names have a higher priority than file extensions.
176-
* @param {!string} id Identifier for this language, use only letters a-z or digits 0-9 and _ inbetween (i.e. "cpp", "foo_bar", "c99")
176+
* @param {!string} id Identifier for this language: lowercase letters and digits with _ separators (e.g. "cpp", "foo_bar", "c99")
177177
* @return {Language} The language with the provided identifier or undefined
178178
*/
179179
function getLanguage(id) {
@@ -320,7 +320,7 @@ define(function (require, exports, module) {
320320

321321
/**
322322
* Sets the identifier for this language or prints an error to the console.
323-
* @param {!string} id Identifier for this language, use only letters a-z or digits 0-9, and _ inbetween (i.e. "cpp", "foo_bar", "c99")
323+
* @param {!string} id Identifier for this language: lowercase letters and digits with _ separators (e.g. "cpp", "foo_bar", "c99")
324324
* @return {boolean} Whether the ID was valid and set or not
325325
*/
326326
Language.prototype._setId = function (id) {
@@ -348,7 +348,7 @@ define(function (require, exports, module) {
348348

349349
/**
350350
* Sets the human-readable name of this language or prints an error to the console.
351-
* @param {!string} name Human-readable name of the language, as it's commonly referred to (i.e. "C++")
351+
* @param {!string} name Human-readable name of the language, as it's commonly referred to (e.g. "C++")
352352
* @return {boolean} Whether the name was valid and set or not
353353
*/
354354
Language.prototype._setName = function (name) {
@@ -371,7 +371,7 @@ define(function (require, exports, module) {
371371
/**
372372
* Loads a mode and sets it for this language.
373373
*
374-
* @param {string|Array.<string>} mode CodeMirror mode (i.e. "htmlmixed"), optionally with a MIME mode defined by that mode ["clike", "text/x-c++src"]
374+
* @param {string|Array.<string>} mode CodeMirror mode (e.g. "htmlmixed"), optionally with a MIME mode defined by that mode ["clike", "text/x-c++src"]
375375
* Unless the mode is located in thirdparty/CodeMirror2/mode/<name>/<name>.js, you need to first load it yourself.
376376
*
377377
* @return {$.Promise} A promise object that will be resolved when the mode is loaded and set
@@ -523,7 +523,7 @@ define(function (require, exports, module) {
523523
/**
524524
* Sets the prefixes to use for line comments in this language or prints an error to the console.
525525
* @param {!string|Array.<string>} prefix Prefix string or and array of prefix strings
526-
* to use for line comments (i.e. "//" or ["//", "#"])
526+
* to use for line comments (e.g. "//" or ["//", "#"])
527527
* @return {boolean} Whether the syntax was valid and set or not
528528
*/
529529
Language.prototype.setLineCommentSyntax = function (prefix) {
@@ -641,13 +641,14 @@ define(function (require, exports, module) {
641641
/**
642642
* Defines a language.
643643
*
644-
* @param {!string} id Unique identifier for this language, use only letters a-z or digits 0-9, and _ inbetween (i.e. "cpp", "foo_bar", "c99")
644+
* @param {!string} id Unique identifier for this language: lowercase letters and digits with _ separators (e.g. "cpp", "foo_bar", "c99")
645645
* @param {!Object} definition An object describing the language
646-
* @param {!string} definition.name Human-readable name of the language, as it's commonly referred to (i.e. "C++")
647-
* @param {Array.<string>} definition.fileExtensions List of file extensions used by this language (i.e. ["php", "php3"])
648-
* @param {Array.<string>} definition.blockComment Array with two entries defining the block comment prefix and suffix (i.e. ["<!--", "-->"])
649-
* @param {string|Array.<string>} definition.lineComment Line comment prefixes (i.e. "//" or ["//", "#"])
650-
* @param {string|Array.<string>} definition.mode CodeMirror mode (i.e. "htmlmixed"), optionally with a MIME mode defined by that mode ["clike", "text/x-c++src"]
646+
* @param {!string} definition.name Human-readable name of the language, as it's commonly referred to (e.g. "C++")
647+
* @param {Array.<string>} definition.fileExtensions List of file extensions used by this language (e.g. ["php", "php3"]). May contain periods (e.g. ["coffee.md"])
648+
* @param {Array.<string>} definition.fileNames List of file names (e.g. ["Makefile"] or ["package.json])
649+
* @param {Array.<string>} definition.blockComment Array with two entries defining the block comment prefix and suffix (e.g. ["<!--", "-->"])
650+
* @param {string|Array.<string>} definition.lineComment Line comment prefixes (e.g. "//" or ["//", "#"])
651+
* @param {string|Array.<string>} definition.mode CodeMirror mode (e.g. "htmlmixed"), optionally with a MIME mode defined by that mode ["clike", "text/x-c++src"]
651652
* Unless the mode is located in thirdparty/CodeMirror2/mode/<name>/<name>.js, you need to first load it yourself.
652653
*
653654
* @return {$.Promise} A promise object that will be resolved with a Language object

‎test/spec/InstallExtensionDialog-test.js

Lines changed: 93 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -495,6 +495,7 @@ define(function (require, exports, module) {
495495
});
496496
});
497497

498+
498499
it("should close when hitting Esc after timing out if cancel doesn't complete quickly", function () {
499500
var deferred = new $.Deferred(),
500501
installer = makeInstaller(null, deferred);
@@ -512,7 +513,20 @@ define(function (require, exports, module) {
512513
});
513514
});
514515

515-
it("should keep close button enabled and not throw an exception if install succeeds after cancelation", function () {
516+
it("should keep close button enabled and not throw an exception if install succeeds quickly after cancelation", function () {
517+
var deferred = new $.Deferred(),
518+
installer = makeInstaller(null, deferred);
519+
runs(function () {
520+
setUrl();
521+
dialog._cancelTimeout = 50;
522+
fields.$okButton.click();
523+
fields.$cancelButton.click();
524+
deferred.resolve();
525+
expect(fields.$okButton.attr("disabled")).toBeFalsy();
526+
});
527+
});
528+
529+
it("should keep close button enabled and not throw an exception if install succeeds slowly after cancelation", function () {
516530
var deferred = new $.Deferred(),
517531
installer = makeInstaller(null, deferred);
518532
runs(function () {
@@ -527,8 +541,36 @@ define(function (require, exports, module) {
527541
expect(fields.$okButton.attr("disabled")).toBeFalsy();
528542
});
529543
});
530-
531-
it("should keep close button enabled and not throw an exception if install fails after cancelation", function () {
544+
it("should keep close button enabled and not throw an exception if install succeeds after cancelation & force close", function () {
545+
var deferred = new $.Deferred(),
546+
installer = makeInstaller(null, deferred);
547+
runs(function () {
548+
setUrl();
549+
dialog._cancelTimeout = 50;
550+
fields.$okButton.click();
551+
fields.$cancelButton.click();
552+
});
553+
waits(100);
554+
runs(function () {
555+
fields.$okButton.click();
556+
deferred.resolve();
557+
expect(fields.$dlg.is(":visible")).toBeFalsy();
558+
});
559+
});
560+
561+
it("should keep close button enabled and not throw an exception if install fails quickly after cancelation", function () {
562+
var deferred = new $.Deferred(),
563+
installer = makeInstaller(null, deferred);
564+
runs(function () {
565+
setUrl();
566+
dialog._cancelTimeout = 50;
567+
fields.$okButton.click();
568+
fields.$cancelButton.click();
569+
deferred.reject();
570+
expect(fields.$okButton.attr("disabled")).toBeFalsy();
571+
});
572+
});
573+
it("should keep close button enabled and not throw an exception if install fails slowly after cancelation", function () {
532574
var deferred = new $.Deferred(),
533575
installer = makeInstaller(null, deferred);
534576
runs(function () {
@@ -544,7 +586,37 @@ define(function (require, exports, module) {
544586
});
545587
});
546588

547-
it("should keep close button enabled and not throw an exception if install cancelation completes after cancelation", function () {
589+
it("should keep close button enabled and not throw an exception if install fails after cancelation & force close", function () {
590+
var deferred = new $.Deferred(),
591+
installer = makeInstaller(null, deferred);
592+
runs(function () {
593+
setUrl();
594+
dialog._cancelTimeout = 50;
595+
fields.$okButton.click();
596+
fields.$cancelButton.click();
597+
});
598+
waits(100);
599+
runs(function () {
600+
fields.$okButton.click();
601+
deferred.reject();
602+
expect(fields.$dlg.is(":visible")).toBeFalsy();
603+
});
604+
});
605+
606+
it("should keep close button enabled and not throw an exception if install cancelation completes quickly after cancelation", function () {
607+
var deferred = new $.Deferred(),
608+
installer = makeInstaller(null, deferred);
609+
runs(function () {
610+
setUrl();
611+
dialog._cancelTimeout = 50;
612+
fields.$okButton.click();
613+
fields.$cancelButton.click();
614+
deferred.reject("CANCELED");
615+
expect(fields.$okButton.attr("disabled")).toBeFalsy();
616+
});
617+
});
618+
619+
it("should keep close button enabled and not throw an exception if install cancelation completes slowly after cancelation", function () {
548620
var deferred = new $.Deferred(),
549621
installer = makeInstaller(null, deferred);
550622
runs(function () {
@@ -558,6 +630,23 @@ define(function (require, exports, module) {
558630
deferred.reject("CANCELED");
559631
expect(fields.$okButton.attr("disabled")).toBeFalsy();
560632
});
633+
});
634+
635+
it("should keep close button enabled and not throw an exception if install cancelation completes after cancelation & force close", function () {
636+
var deferred = new $.Deferred(),
637+
installer = makeInstaller(null, deferred);
638+
runs(function () {
639+
setUrl();
640+
dialog._cancelTimeout = 50;
641+
fields.$okButton.click();
642+
fields.$cancelButton.click();
643+
});
644+
waits(100);
645+
runs(function () {
646+
fields.$okButton.click();
647+
deferred.reject("CANCELED");
648+
expect(fields.$dlg.is(":visible")).toBeFalsy();
649+
});
561650

562651
// This must be in the last spec in the suite.
563652
runs(function () {

‎test/spec/LanguageManager-test.js

Lines changed: 1 addition & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -386,48 +386,14 @@ define(function (require, exports, module) {
386386
it("should update the document's language when a file is renamed", function () {
387387
var javascript = LanguageManager.getLanguage("javascript"),
388388
html = LanguageManager.getLanguage("html"),
389-
doc = SpecRunnerUtils.createMockDocument("foo", javascript),
389+
doc = SpecRunnerUtils.createMockActiveDocument({ filename: "foo.js", language: "javascript" }),
390390
spy = jasmine.createSpy("languageChanged event handler");
391391

392392
// sanity check language
393393
expect(doc.getLanguage()).toBe(javascript);
394394

395395
// Documents are only 'active' while referenced; they won't be maintained by DocumentManager
396396
// for global updates like rename otherwise.
397-
// Undo createMockDocument()'s shimming to allow this.
398-
doc.addRef = DocumentManager.Document.prototype.addRef;
399-
doc.releaseRef = DocumentManager.Document.prototype.releaseRef;
400-
doc.addRef();
401-
402-
// listen for event
403-
$(doc).on("languageChanged", spy);
404-
405-
// trigger a rename
406-
DocumentManager.notifyPathNameChanged(doc.file.name, "dummy.html", false);
407-
408-
// language should change
409-
expect(doc.getLanguage()).toBe(html);
410-
expect(spy).toHaveBeenCalled();
411-
expect(spy.callCount).toEqual(1);
412-
413-
// check callback args (arg 0 is a jQuery event)
414-
expect(spy.mostRecentCall.args[1]).toBe(javascript);
415-
expect(spy.mostRecentCall.args[2]).toBe(html);
416-
417-
// cleanup
418-
doc.releaseRef();
419-
});
420-
421-
it("should update the document's language when a language is added", function () {
422-
var javascript = LanguageManager.getLanguage("javascript"),
423-
html = LanguageManager.getLanguage("html"),
424-
doc = SpecRunnerUtils.createMockActiveDocument({ filename: "foo.js", language: "javascript" }),
425-
spy = jasmine.createSpy("languageChanged event handler");
426-
427-
// sanity check language
428-
expect(doc.getLanguage()).toBe(javascript);
429-
430-
// make active
431397
doc.addRef();
432398

433399
// listen for event

‎test/spec/SpecRunnerUtils.js

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -142,8 +142,14 @@ define(function (require, exports, module) {
142142
};
143143

144144
/**
145-
* Returns a Document suitable for use with an Editor in isolation, but
146-
* maintained active for global updates like name and language changes.
145+
* Returns a Document suitable for use with an Editor in isolation, but that can be registered with
146+
* DocumentManager via addRef() so it is maintained for global updates like name and language changes.
147+
*
148+
* Like a normal Document, if you cause an addRef() on this you MUST call releaseRef() later.
149+
*
150+
* @param {!{language:?string, filename:?string, content:?string }} options
151+
* Language defaults to JavaScript, filename defaults to a placeholder name, and
152+
* content defaults to "".
147153
*/
148154
function createMockActiveDocument(options) {
149155
var language = options.language || LanguageManager.getLanguage("javascript"),
@@ -173,6 +179,11 @@ define(function (require, exports, module) {
173179
/**
174180
* Returns a Document suitable for use with an Editor in isolation: i.e., a Document that will
175181
* never be set as the currentDocument or added to the working set.
182+
*
183+
* Unlike a real Document, does NOT need to be explicitly cleaned up.
184+
*
185+
* @param {?string} initialContent Defaults to ""
186+
* @param {?string} languageId Defaults to JavaScript
176187
*/
177188
function createMockDocument(initialContent, languageId) {
178189
var language = LanguageManager.getLanguage(languageId) || LanguageManager.getLanguage("javascript"),
@@ -183,14 +194,23 @@ define(function (require, exports, module) {
183194
// fails to clean up properly (if test fails, or due to an apparent bug with afterEach())
184195
docToShim.addRef = function () {};
185196
docToShim.releaseRef = function () {};
197+
docToShim._ensureMasterEditor = function () {
198+
if (!this._masterEditor) {
199+
// Don't let Document create an Editor itself via EditorManager; the unit test can't clean that up
200+
throw new Error("Use create/destroyMockEditor() to test edit operations");
201+
}
202+
};
186203

187204
return docToShim;
188205
}
189206

190207
/**
191-
* Returns a Document and Editor suitable for use with an Editor in
192-
* isolation: i.e., a Document that will never be set as the
193-
* currentDocument or added to the working set.
208+
* Returns a Document and Editor suitable for use with an Editor in isolation: i.e., a
209+
* Document that will never be set as the currentDocument or added to the working set.
210+
* The Editor *will* be reported as the "active editor" by EditorManager, however.
211+
*
212+
* Must be cleaned up by calling destroyMockEditor(document) later.
213+
*
194214
* @return {!{doc:{Document}, editor:{Editor}}}
195215
*/
196216
function createMockEditor(initialContent, languageId, visibleRange) {
@@ -216,9 +236,10 @@ define(function (require, exports, module) {
216236

217237
/**
218238
* Destroy the Editor instance for a given mock Document.
219-
* @param {!Document} doc
239+
* @param {!Document} doc Document whose master editor to destroy
220240
*/
221241
function destroyMockEditor(doc) {
242+
EditorManager._notifyActiveEditorChanged(null);
222243
EditorManager._destroyEditorIfUnneeded(doc);
223244

224245
// Clear editor holder so EditorManager doesn't try to resize destroyed object

‎test/spec/StringMatch-test.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -639,14 +639,20 @@ define(function (require, exports, module) {
639639
// Object.prototype has toString
640640
var toStringResult = matcher.match("toString", "t");
641641
expect(toStringResult).toBeTruthy();
642+
toStringResult = matcher.match("toString", "x");
643+
expect(toStringResult).toBeFalsy();
642644

643645
// Array.prototype has length
644646
var lengthResult = matcher.match("length", "l");
645647
expect(lengthResult).toBeTruthy();
648+
lengthResult = matcher.match("length", "x");
649+
expect(lengthResult).toBeFalsy();
646650

647651
// Object.prototype has hasOwnProperty
648652
var hasOwnPropertyResult = matcher.match("hasOwnProperty", "h");
649653
expect(hasOwnPropertyResult).toBeTruthy();
654+
hasOwnPropertyResult = matcher.match("hasOwnProperty", "x");
655+
expect(hasOwnPropertyResult).toBeFalsy();
650656
});
651657

652658
it("can reset the caches", function () {

0 commit comments

Comments
 (0)