Repository navigation
Allow plugins to turn off the “link to last URL” navigation helper - #13044
Conversation
cjcenizal
left a comment
There was a problem hiding this comment.
Sweet, sweet tests. 🤤 Had a few comments and would like to hear your thoughts!
There was a problem hiding this comment.
Such a nit-pick but the term "built-in" isn't necessary here right? 🤓
There was a problem hiding this comment.
Or in the next two tests.
There was a problem hiding this comment.
If it's in question, then probably not :)
There was a problem hiding this comment.
Do we need all of these assertions here? If one of them fails and the message is "expected true to be false", it will be tricky to figure out what went wrong. If we need all of these assertions, maybe we can split them into two tests, one for "has listed set to true if it's passed as true" and another for "has listed set to false if it's passed as false"?
There was a problem hiding this comment.
I disagree that it is tricky to figure out what went wrong if there is a failure because the failure will have a stack trace that points to the failing assertion. In the past I have went through some hoops to avoid that kind of failure messaging, but eventually just simplified because my team member and myself felt it wasn't worth the extra code.
There was a problem hiding this comment.
Doesn't the stack trace solve this issue though? Just trace to the line of the failure.
There was a problem hiding this comment.
FYI, I'm not exactly sure why Github says this is an outdated change. I didn't explicitly make any change here.
There was a problem hiding this comment.
That's so funny, I didn't even know about the stack trace. 😄 Yes that addresses my concern.
There was a problem hiding this comment.
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', () => {
describe('defaults to the opposite value of hidden', () => {
it(`when it's null and 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(`dhen it's null and 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 it's undefined and hidden is false`, () => {
const spec = {
id: 'uiapp-test',
hidden: false,
};
const newApp = new UiApp(uiExports, spec);
expect(newApp.listed).to.be(true);
});
it(`when it's undefined and hidden is true`, () => {
const spec = {
id: 'uiapp-test',
hidden: true,
};
const newApp = new UiApp(uiExports, spec);
expect(newApp.listed).to.be(false);
});
});
it(`is set to true when it's passed as true`, () => {
const spec = {
id: 'uiapp-test',
listed: true,
};
const newApp = new UiApp(uiExports, spec);
expect(newApp.listed).to.be(true);
});
it(`is set to false when it's passed as false`, () => {
const spec = {
id: 'uiapp-test',
listed: false,
};
const newApp = new UiApp(uiExports, spec);
expect(newApp.listed).to.be(false);
});
});There was a problem hiding this comment.
I was also a little surprised that these properties have a relationship and the logic previously had no test coverage. But I think if it is hard to figure that out just by looking at the test, would it suffice to just add a comment in the code that all these tests are here to check the logic for the relationship?
Your proposal looks good to me though, and I appreciate you taking the time to present it like that. I have no problem with integrating it. I'll add a comment as well.
|
Really weird test failure! |
c17bf54 to
25070cf
Compare
* [App] Allow plugin app to specify linkToLastUrl * clean up tests * remove "built-in" wording * restructure test to clarify the behavior
This re-implements #9011 for 6.0