Conversation
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.
Nice work @JeremiePat! This is nicely organized and thorough. I have requested changes, but the only one that's required is the addition of the stub for wiki.pageExists
. You can do with the whitespace comments as you wish.
Thanks @JeremiePat!
tests/macros/test-svginfo.js
Outdated
const SVG_BASE_SLUG = 'docs/Web/SVG'; | ||
const SVG_DATA = require('../../macros/SVGData.json'); | ||
const L10N_COMMON = require('../../macros/L10n-Common.json'); | ||
const L10N_SVG = require('../../macros/L10n-SVG.json'); |
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 don't think we follow a JavaScript style guide yet (@schalkneethling is working on one), but I would prefer each =
in lines 14-16 to be preceded by only a single space (and not aligned as they are).
tests/macros/test-svginfo.js
Outdated
const SEPARATOR = _('listSeparator', locale); | ||
const CATEGORIES = _('categories', locale); | ||
const CONTENT = _('permittedContent', locale); | ||
const DESCRIPTION = _(data.content.description, locale); |
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.
Another small whitespace thing again, but I would prefer only a single space before each =
and after each ,
.
tests/macros/test-svginfo.js
Outdated
output: makeExpect(SVG_DATA.elements.defs) | ||
},{ | ||
// altGlyphDef allows to test when description is an object rather than a string | ||
title: 'Test implicite slug (no param, slug: /en-US/docs/Web/SVG/Element/altGlyphDef)', |
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.
Spelling: implicite
should be implicit
tests/macros/test-svginfo.js
Outdated
output: '<span style="color:red;">missing</span>' | ||
},{ | ||
// Defs allows to test multiple category and a mix of groups and elements for permitted contents | ||
title: 'Test explicite slug (param: defs)', |
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.
Spelling: explicite
should be explicit
tests/macros/test-svginfo.js
Outdated
slug: URL('en-US', SVG_BASE_SLUG, 'Element', 'altGlyphDef') | ||
} | ||
},{ | ||
title: 'Test implicite non English slug (no param, slug: /zh-CN/docs/Web/SVG/Element/defs)', |
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.
Spelling: implicite
should be implicit
// | ||
// NOTE: we could probably make that more generic by having a single test | ||
// runner (see below) and a bunch of JSON files (one per macro) to | ||
// describe all the possible inputs and their expected outputs. |
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.
On further reflection, in order for this approach to be used for more general testing, the definition of each test case would also have to include its beforeEachMacro
set-up (creating and populating the stubs for example), which seems difficult to standardize.
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.
Indeed, at that point I need to get into more testing to see what can be standardize (and how) and what cannot and how is it possible to neatly handle special edge case. Those notes are here to remember to have further discussion when we will have more tests.
tests/macros/test-svginfo.js
Outdated
const { url, data } = MOCK_PAGES['zh-CN'][key]; | ||
macro.ctx.wiki.getPage.withArgs(url).returns(data); | ||
}); | ||
}); |
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.
This is nicely done, but it's missing the stub for the wiki.pageExists
call that is made within the SVGElement
macro (via the call to template("SVGElement"...
made in one of the paths through the svginfo
macro). The way the test runs currently, it's making an actual network request via the real wiki.pageExists
. Something like this worked for me during my testing:
beforeEachMacro((macro) => {
// let's make sure we have clean calls to wiki.getPage
macro.ctx.wiki.getPage = sinon.stub();
macro.ctx.wiki.pageExists = sinon.stub();
Object.keys(MOCK_PAGES['en-US']).forEach((key) => {
const { url, data } = MOCK_PAGES['en-US'][key];
macro.ctx.wiki.getPage.withArgs(url).returns(data);
macro.ctx.wiki.pageExists.withArgs(url).returns(true);
});
Object.keys(MOCK_PAGES['zh-CN']).forEach((key) => {
const { url, data } = MOCK_PAGES['zh-CN'][key];
macro.ctx.wiki.getPage.withArgs(url).returns(data);
macro.ctx.wiki.pageExists.withArgs(url).returns(true);
});
});
This reminds me that I need to submit a PR that stubs the request
package within the macro.ctx
object, so that it throws an error when called. I think that would be the best way to catch all of the attempts to make real network calls, and help us discover what we need to stub.
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.
It's been a while since I've worked with the test framework, as I forgot that I had already stubbed the request
package long ago in #240! 🤦♂️
So your tests were not making real network calls, sorry! I still think it's best to create and populate the stub for wiki.pageExists
instead of relying on whatever wiki.pageExists
returns using the un-populated request
stub.
tests/macros/test-svginfo.js
Outdated
let { elements, groups } = data.content.elements.reduce((acc, value) => { | ||
if (value.indexOf('<') !== -1) { | ||
let key = value.replace(/<|>/g, ''); | ||
let url = URL(locale, SVG_BASE_SLUG, 'Element', key); |
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.
Whitespace again: I would prefer only a single space before each =
.
tests/macros/test-svginfo.js
Outdated
} else { | ||
let anchor = "#" + camelToSnake(value); | ||
let url = URL(locale, SVG_BASE_SLUG, 'Element') + anchor; | ||
let label = _(value, locale); |
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.
Whitespace again: I would prefer only a single space before each =
.
tests/macros/test-svginfo.js
Outdated
}, {elements: [], groups: []}); | ||
|
||
// Named groups must be listed first (each on new lines) | ||
if (groups.length > 0) permittedContent.push(groups.join('<br/>')); |
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.
Whitespace again: I would prefer only a single space before the >
.
It should be better now. For For the white space, I made the changes even if I'm someone who is very sensitive to vertical alignment but I don't want to fight on this. |
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.
Thanks @JeremiePat, this looks great, thanks again for creating these new comprehensive tests for the svginfo
macro! Feel free to ignore my whitespace preferences in the future! 😃
Here are some test for the current state of {{svginfo}}
They cover: