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

Reactivate HTML Menu unit tests as part of the test suite #5290

Merged
redmunds merged 4 commits into from
Sep 26, 2013
Merged

Reactivate HTML Menu unit tests as part of the test suite #5290

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3f3712c
Split menu specs into HTML and Native. Add HTML and Native versions …
lkcampbell Sep 22, 2013
1db93ab
Set brackets.nativeMenus based on hasNativeMenus URL parameter.
lkcampbell Sep 23, 2013
a9a8e47
API changes
lkcampbell Sep 25, 2013
54e3ad0
hasNativeMenus param code cleanup
lkcampbell Sep 26, 2013
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
14 changes: 12 additions & 2 deletions src/utils/Global.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,13 @@
define(function (require, exports, module) {
"use strict";

var configJSON = require("text!config.json");
var configJSON = require("text!config.json"),
UrlParams = require("utils/UrlParams").UrlParams;

var params = new UrlParams();

// read URL params
params.parse();

// Define core brackets namespace if it isn't already defined
//
Expand Down Expand Up @@ -77,7 +83,11 @@ define(function (require, exports, module) {

global.brackets.inBrowser = !global.brackets.hasOwnProperty("fs");

global.brackets.nativeMenus = (!global.brackets.inBrowser && (global.brackets.platform !== "linux"));
if (params.get("hasNativeMenus") !== undefined) {
global.brackets.nativeMenus = (params.get("hasNativeMenus") === "true");
} else {
global.brackets.nativeMenus = (!global.brackets.inBrowser && (global.brackets.platform !== "linux"));
}

Copy link
Copy Markdown
Contributor

@redmunds redmunds Sep 25, 2013

Choose a reason for hiding this comment

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

This form is simpler, and it also checks for null and empty string:

    if (params.get("hasNativeMenus")) {
        global.brackets.nativeMenus = (params.get("hasNativeMenus") === "true");
    } else {
        global.brackets.nativeMenus = (!global.brackets.inBrowser && (global.brackets.platform !== "linux"));
    }

Also, how expensive is the call to params.get("hasNativeMenus")? Should it be stored in a var so it's not called twice?

global.brackets.isLocaleDefault = function () {
return !global.localStorage.getItem("locale");
Expand Down
44 changes: 36 additions & 8 deletions test/spec/Menu-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,16 @@ define(function (require, exports, module) {
KeyEvent = require("utils/KeyEvent");


describe("Menus", function () {
describe("Menus (Native Shell)", function () {

this.category = "integration";

var testWindow;

beforeFirst(function () {
// Create a new window that will be shared by ALL tests in this spec. (We need the tests to
// run in a real Brackets window since HTMLCodeHints requires various core modules (it can't
// run 100% in isolation), but popping a new window per testcase is unneeded overhead).
var testWindowOptions = {"hasNativeMenus" : true};

// Create a new native menu window that will be shared by ALL tests in this spec.
SpecRunnerUtils.createTestWindowAndRun(this, function (w) {
testWindow = w;

Expand All @@ -54,7 +54,7 @@ define(function (require, exports, module) {
Commands = testWindow.brackets.test.Commands;
KeyBindingManager = testWindow.brackets.test.KeyBindingManager;
Menus = testWindow.brackets.test.Menus;
});
}, testWindowOptions);
});

afterLast(function () {
Expand Down Expand Up @@ -210,11 +210,39 @@ define(function (require, exports, module) {
expect($menus.length).toBe(0);
});
});
});


describe("Menus (HTML)", function () {

this.category = "integration";

var testWindow;

if (!brackets.inBrowser) {
return;
}
beforeFirst(function () {
var testWindowOptions = {"hasNativeMenus" : false};

// Create a new HTML menu window that will be shared by ALL tests in this spec.
SpecRunnerUtils.createTestWindowAndRun(this, function (w) {
testWindow = w;

// Load module instances from brackets.test
CommandManager = testWindow.brackets.test.CommandManager;
Commands = testWindow.brackets.test.Commands;
KeyBindingManager = testWindow.brackets.test.KeyBindingManager;
Menus = testWindow.brackets.test.Menus;
}, testWindowOptions);
});

afterLast(function () {
testWindow = null;
CommandManager = null;
Commands = null;
KeyBindingManager = null;
Menus = null;
SpecRunnerUtils.closeTestWindow();
});

describe("Add Menus", function () {

function getTopMenus() {
Expand Down
10 changes: 7 additions & 3 deletions test/spec/SpecRunnerUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,8 +476,7 @@ define(function (require, exports, module) {
waitsForDone(promise, "dismiss dialog");
}


function createTestWindowAndRun(spec, callback) {
function createTestWindowAndRun(spec, callback, options) {
runs(function () {
// Position popup windows in the lower right so they're out of the way
var testWindowWid = 1000,
Expand All @@ -503,6 +502,11 @@ define(function (require, exports, module) {
// disable initial dialog for live development
params.put("skipLiveDevelopmentInfo", true);

// option to launch test window with either native or HTML menus
if (options && options.hasOwnProperty("hasNativeMenus")) {
params.put("hasNativeMenus", options.hasNativeMenus);
Copy link
Copy Markdown
Contributor

@redmunds redmunds Sep 25, 2013

Choose a reason for hiding this comment

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

Later, params.get() compares result as a string (not a boolean) value, so I think params.put() should pass it as a string:

    params.put("hasNativeMenus", (options.hasNativeMenus ? "true" : "false"));

Copy link
Copy Markdown
Contributor Author

@lkcampbell lkcampbell Sep 26, 2013

Choose a reason for hiding this comment

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

Agreed. These changes are much clearer. The code got a bit convoluted when I was wrestling with the false boolean value versus the truthy "false" string.

}

_testWindow = window.open(getBracketsSourceRoot() + "/index.html?" + params.toString(), "_blank", optionsStr);

_testWindow.isBracketsTestWindow = true;
Expand All @@ -524,7 +528,7 @@ define(function (require, exports, module) {
});
};
});

// FIXME (issue #249): Need an event or something a little more reliable...
waitsFor(
function isBracketsDoneLoading() {
Expand Down
⚡