-
Notifications
You must be signed in to change notification settings - Fork 8.5k
Allow plugins to turn off the “link to last URL” navigation helper #9011
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,263 @@ | ||
| import expect from 'expect.js'; | ||
| import UiApp from '../ui_app.js'; | ||
| import UiExports from '../ui_exports'; | ||
| import { isUndefined, noop } from 'lodash'; | ||
|
|
||
| function getMockSpec(extraParams) { | ||
| return { | ||
| id: 'uiapp-test', | ||
| main: 'main.js', | ||
| title: 'UIApp Test', | ||
| order: 9000, | ||
| description: 'Test of UI App Constructor', | ||
| icon: 'ui_app_test.svg', | ||
| linkToLastSubUrl: true, | ||
| hidden: false, | ||
| listed: null, | ||
| templateName: 'ui_app_test', | ||
| ...extraParams | ||
| }; | ||
| } | ||
| describe('UiApp', () => { | ||
| describe('constructor', () => { | ||
| const uiExports = new UiExports({}); | ||
|
|
||
| it('throw if ID is not given', () => { | ||
| function newAppMissingID() { | ||
| const spec = {}; // should have id property | ||
| const newApp = new UiApp(uiExports, spec); | ||
| return newApp; | ||
| } | ||
| expect(newAppMissingID).to.throwException(); | ||
| }); | ||
|
|
||
| describe('defaults', () => { | ||
| const spec = { id: 'uiapp-test-defaults' }; | ||
| let newApp; | ||
| beforeEach(() => { | ||
| newApp = new UiApp(uiExports, spec); | ||
| }); | ||
|
|
||
| it('use the spec ID', () => { | ||
| expect(newApp.id).to.be('uiapp-test-defaults'); | ||
| }); | ||
|
|
||
| it('creates fields with undefined value', () => { | ||
| // test that the fields exist, but have undefined value | ||
| expect('main' in newApp).to.be(true); | ||
| expect(isUndefined(newApp.main)).to.be(true); | ||
|
|
||
| expect('title' in newApp).to.be(true); | ||
| expect(isUndefined(newApp.title)).to.be(true); | ||
|
|
||
| expect('description' in newApp).to.be(true); | ||
| expect(isUndefined(newApp.description)).to.be(true); | ||
|
|
||
| expect('icon' in newApp).to.be(true); | ||
| expect(isUndefined(newApp.icon)).to.be(true); | ||
|
|
||
| expect('linkToLastSubUrl' in newApp).to.be(true); | ||
| expect(isUndefined(newApp.linkToLastSubUrl)).to.be(true); | ||
| }); | ||
|
|
||
| it('default navLink', () => { | ||
| expect(newApp.navLink).to.eql({ | ||
| id: 'uiapp-test-defaults', | ||
| title: undefined, | ||
| order: 0, | ||
| url: '/app/uiapp-test-defaults', | ||
| description: undefined, | ||
| icon: undefined, | ||
| linkToLastSubUrl: true, | ||
| hidden: false, | ||
| disabled: false, | ||
| tooltip: '' | ||
| }); | ||
| }); | ||
|
|
||
| it('default order of 0', () => { | ||
| expect(newApp.order).to.be(0); | ||
| }); | ||
|
|
||
| it('template name is ui_app', () => { | ||
| expect(newApp.templateName).to.be('ui_app'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('with spec', () => { | ||
| const spec = getMockSpec(); | ||
| let newApp; | ||
| beforeEach(() => { | ||
| newApp = new UiApp(uiExports, spec); | ||
| }); | ||
|
|
||
| it('use the spec ID', () => { | ||
| expect(newApp.id).to.be('uiapp-test'); | ||
| }); | ||
|
|
||
| it('copies field values from spec', () => { | ||
| // test that the fields exist, but have undefined value | ||
| expect(newApp.main).to.be('main.js'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small suggestion, but we can make these tests a bit less brittle by asserting against a reference to the spec here, e.g. expect(newApp.main).to.be(spec.main); |
||
| expect(newApp.title).to.be('UIApp Test'); | ||
| expect(newApp.description).to.be('Test of UI App Constructor'); | ||
| expect(newApp.icon).to.be('ui_app_test.svg'); | ||
| expect(newApp.linkToLastSubUrl).to.be(true); | ||
| expect(newApp.templateName).to.be('ui_app_test'); | ||
| expect(newApp.order).to.be(9000); | ||
| expect(newApp.navLink).to.eql({ | ||
| id: 'uiapp-test', | ||
| title: 'UIApp Test', | ||
| order: 9000, | ||
| url: '/app/uiapp-test', | ||
| description: 'Test of UI App Constructor', | ||
| icon: 'ui_app_test.svg', | ||
| linkToLastSubUrl: true, | ||
| hidden: false, | ||
| disabled: false, | ||
| tooltip: '' | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('reference fields', () => { | ||
| const spec = getMockSpec({ testSpec: true }); | ||
| let newApp; | ||
| beforeEach(() => { | ||
| newApp = new UiApp(uiExports, spec); | ||
| }); | ||
|
|
||
| it('uiExports', () => { | ||
| expect(newApp.uiExports).to.be(uiExports); | ||
| }); | ||
| it('spec', () => { | ||
| expect(newApp.spec).to.be(spec); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getInjectedVars', () => { | ||
| it('noop function by default', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another small suggestion, but I like to make sure the message in the terminal forms a grammatically-correct sentence, e.g. by changing this to 'is noop function by default', we will get the message: vs |
||
| const spec = { | ||
| id: 'uiapp-test' | ||
| }; | ||
| const newApp = new UiApp(uiExports, spec); | ||
| expect(newApp.getInjectedVars).to.be(noop); | ||
| }); | ||
| it('reference to spec.injectVars', () => { | ||
| const helloFunction = () => 'hello'; | ||
| const spec = { | ||
| id: 'uiapp-test', | ||
| injectVars: helloFunction | ||
| }; | ||
| const newApp = new UiApp(uiExports, spec); | ||
| expect(newApp.getInjectedVars).to.be(helloFunction); | ||
| }); | ||
| }); | ||
|
|
||
| describe('hidden and listed', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I was reading these tests, I was having a hard time figuring out how these two properties interacted. After I read the source, I saw that listed was dependent on hidden, and that hidden was cast to a boolean values. I think this structure makes that relationship a little clearer. What do you think? describe('hidden', () => {
describe('is cast to boolean value', () => {
it('when undefined', () => {
const spec = {
id: 'uiapp-test',
};
const newApp = new UiApp(uiExports, spec);
expect(newApp.hidden).to.be(false);
});
it('when null', () => {
const spec = {
id: 'uiapp-test',
hidden: null,
};
const newApp = new UiApp(uiExports, spec);
expect(newApp.hidden).to.be(false);
});
});
});
describe('listed', () => {
it('when null defaults to the opposite value of hidden, when hidden is true', () => {
const spec = {
id: 'uiapp-test',
hidden: true,
listed: null,
};
const newApp = new UiApp(uiExports, spec);
expect(newApp.listed).to.be(false);
});
it('when null defaults to the opposite value of hidden, when hidden is false', () => {
const spec = {
id: 'uiapp-test',
hidden: false,
listed: null,
};
const newApp = new UiApp(uiExports, spec);
expect(newApp.listed).to.be(true);
});
it('when undefined defaults to the opposite value of hidden, when hidden is false', () => {
const spec = {
id: 'uiapp-test',
hidden: false,
};
const newApp = new UiApp(uiExports, spec);
expect(newApp.listed).to.be(true);
});
it('when undefined defaults to the opposite value of hidden, when hidden is true', () => {
const spec = {
id: 'uiapp-test',
hidden: true,
};
const newApp = new UiApp(uiExports, spec);
expect(newApp.listed).to.be(false);
});
it('when true is set to true', () => {
const spec = {
id: 'uiapp-test',
listed: true,
};
const newApp = new UiApp(uiExports, spec);
expect(newApp.listed).to.be(true);
});
it('when false is set to false', () => {
const spec = {
id: 'uiapp-test',
listed: false,
};
const newApp = new UiApp(uiExports, spec);
expect(newApp.listed).to.be(false);
});
}); |
||
| it('if hidden and listed are not set, hidden is set as false and listed is set as true', () => { | ||
| const spec = { | ||
| id: 'uiapp-test' | ||
| }; | ||
| const newApp = new UiApp(uiExports, spec); | ||
| expect(newApp.hidden).to.be(false); | ||
| expect(newApp.listed).to.be(true); | ||
| }); | ||
| it('if listed is passed as null, and hidden is true, listed is set as false', () => { | ||
| const spec = { | ||
| id: 'uiapp-test', | ||
| hidden: true, | ||
| listed: null | ||
| }; | ||
| const newApp = new UiApp(uiExports, spec); | ||
| expect(newApp.hidden).to.be(true); | ||
| expect(newApp.listed).to.be(false); | ||
| }); | ||
| it('if listed is passed as null, and hidden is false, listed is set as true', () => { | ||
| const spec = { | ||
| id: 'uiapp-test', | ||
| hidden: false, | ||
| listed: null | ||
| }; | ||
| const newApp = new UiApp(uiExports, spec); | ||
| expect(newApp.hidden).to.be(false); | ||
| expect(newApp.listed).to.be(true); | ||
| }); | ||
| it('if listed is passed as null, and hidden not set, listed is set as true', () => { | ||
| const spec = { | ||
| id: 'uiapp-test', | ||
| listed: null | ||
| }; | ||
| const newApp = new UiApp(uiExports, spec); | ||
| expect(newApp.hidden).to.be(false); | ||
| expect(newApp.listed).to.be(true); | ||
| }); | ||
| it('if listed is passed as true, is set with that value', () => { | ||
| const spec = { | ||
| id: 'uiapp-test', | ||
| listed: true | ||
| }; | ||
| const newApp = new UiApp(uiExports, spec); | ||
| expect(newApp.hidden).to.be(false); | ||
| expect(newApp.listed).to.be(true); | ||
| }); | ||
| it('if listed is passed as false, is set with that value', () => { | ||
| const spec = { | ||
| id: 'uiapp-test', | ||
| listed: false | ||
| }; | ||
| const newApp = new UiApp(uiExports, spec); | ||
| expect(newApp.hidden).to.be(false); | ||
| expect(newApp.listed).to.be(false); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getModules', () => { | ||
| it('gets modules from uiExports', () => { | ||
| const uiExports = new UiExports({}); | ||
| uiExports.consumePlugin({ | ||
| uiExportsSpecs: { | ||
| chromeNavControls: [ 'plugins/ui_app_test/views/nav_control' ], | ||
| hacks: [ 'plugins/ui_app_test/hacks/init' ] | ||
| } | ||
| }); | ||
| const spec = getMockSpec(); | ||
| const newApp = new UiApp(uiExports, spec); | ||
|
|
||
| expect(newApp.getModules()).to.eql([ | ||
| 'main.js', | ||
| 'plugins/ui_app_test/views/nav_control', | ||
| 'plugins/ui_app_test/hacks/init' | ||
| ]); | ||
| }); | ||
| }); | ||
|
|
||
| describe('toJSON', function () { | ||
| it('creates plain object', () => { | ||
| const uiExports = new UiExports({}); | ||
| const spec = getMockSpec(); | ||
| const newApp = new UiApp(uiExports, spec); | ||
|
|
||
| expect(newApp.toJSON()).to.eql({ | ||
| id: 'uiapp-test', | ||
| title: 'UIApp Test', | ||
| description: 'Test of UI App Constructor', | ||
| icon: 'ui_app_test.svg', | ||
| main: 'main.js', | ||
| navLink: { | ||
| id: 'uiapp-test', | ||
| title: 'UIApp Test', | ||
| order: 9000, | ||
| url: '/app/uiapp-test', | ||
| description: 'Test of UI App Constructor', | ||
| icon: 'ui_app_test.svg', | ||
| linkToLastSubUrl: true, | ||
| hidden: false, | ||
| disabled: false, | ||
| tooltip: '' | ||
| }, | ||
| linkToLastSubUrl: true | ||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make these assert against
undefinedinstead?I think this will improve the messages we see in the terminal. As the tests currently are, they will say something like "expected true but got false", which won't give you any insight into what the actual value of the property was. If we change it, the message will be something like "expected undefined but got 'value'".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deliberately didn't do that because it wouldn't actually assure anything. In JS, a field that was never set on an object evaluates to
undefinedThe
inoperator was the only way I know of to test that a field has actually been set:Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the docs they give an example making an assertion that something is
undefined:I think all that matters in this case is that the default values of various properties is
undefinedwhen they're undefined by thespecargument. It doesn't matter whether the internal implementation is something likeor