Add tests for DefaultAPISidebar #1135
Conversation
| const commonl10nFixture = fs.readFileSync(commonl10nFixturePath, 'utf8'); | ||
| const commonL10nJSON = JSON.parse(commonl10nFixture); | ||
| const groupDataFixturePath = path.resolve(__dirname, 'fixtures/defaultapisidebar/groupdata.json'); | ||
| const groupDataFixture = fs.readFileSync(groupDataFixturePath, 'utf8'); |
There was a problem hiding this comment.
I’d prefer if this used readFixture(…) from #1093.
| macro.ctx.env.slug = 'not undefined'; | ||
| // Mock calls to L10n-Common and GroupData | ||
| const originalTemplate = macro.ctx.template; | ||
| macro.ctx.template = jest.fn( async (name, ...args) => { |
There was a problem hiding this comment.
I’d prefer if this used macro.mockTemplate(…) from #1093.
|
I've renamed the test script to match the macro (I assume that's what you meant). Makes sense. I can't use |
| @@ -0,0 +1,75 @@ | |||
| { | |||
There was a problem hiding this comment.
Just a small point that it looks like only 7 of the 12 keys are actually used by the DefaultAPISidebar macro (Methods, Properties, Events, Interfaces, Guides, [Translate], and TranslationCTA), so this file could be smaller if desired.
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const subpagesFixturePath = path.resolve(__dirname, 'fixtures/defaultapisidebar/subpages.json'); | ||
| const subpagesFixture = JSON.parse(fs.readFileSync(subpagesFixturePath, 'utf8')); |
There was a problem hiding this comment.
I think subpagesJSON rather than subpagesFixture would be a clearer name and more consistent with your naming scheme?
| _next = checkSubList('Interfaces', config, details, checkItem, _next); | ||
| _next = checkSubList('Properties', config, details, checkItem, _next); | ||
| _next = checkSubList('Methods', config, details, checkItem, _next); | ||
| _next = checkSubList('Events', config, details, checkItem, _next); |
There was a problem hiding this comment.
Super nit: The _ could be removed if this last variable assignment wasn't done, so this would work as well:
let next = 0;
next = checkSubList('Guides', config, details, checkGuideItem, next);
next = checkSubList('Interfaces', config, details, checkItem, next);
next = checkSubList('Properties', config, details, checkItem, next);
next = checkSubList('Methods', config, details, checkItem, next);
checkSubList('Events', config, details, checkItem, next);
but that's a personal style preference.
|
Thanks for the review @escattone . All your suggestions seem like good improvements 👍 |
…events * upstream/master: Add tests for DefaultAPISidebar (mdn#1135) Delete a11y Gecko macros (mdn#1144) Delete User xref macro (mdn#1143) Updated to account for corrected article title and slug, and move to the user_interface folder to align with the other user interface articles Added "users" into the menu title, to align with the page title. Accessibility guidelines: Browser extension sidebar menu update rm eventref and test Browser extension sidebar update: Data and privacy consent prompts article fix(macros): Use `<code class=templateLink>` for {{TemplateLink}}
This PR is part of mdn/sprints#1314. It adds tests for the DefaultAPISidebar macro in preparation for updating the macro to handle the new way we are organizing event reference documentation.
For some more detail on how this macro works and when it is supposed to be used (AFAICT), please see mdn/sprints#1314 (comment).
We test with several different "GroupData" structures, that feature different combinations of items: missing arrays, empty arrays, arrays containing one or more entry.
Note that guides may be specified in two places:
We test for this using various GroupData structures in combination with non-empty and empty "subpages" structures.
If the same guide appears in GroupData.guides and also as a subpage, then it gets listed twice. This looks like a bug, but for now I've chosen to keep the current behavior and test for it, rather than fix it, because I'm not sure of the best way to fix it.
One fix I have made now is: the "overview" entry in GroupData is optional. If it's not present, then the macro is not able to look in subpages for guides (because subpages is relative to the overview page). However in the current implementation, if "overview" is missing then the macro also ignores GroupData.guides (https://github.com/mdn/kumascript/blob/master/macros/DefaultAPISidebar.ejs#L43-L44). This just seems wrong, so I've changed it.