Skip to content

Commit

Permalink
πŸ—β™»οΈπŸš€ Updates to app-index tests (#34887)
Browse files Browse the repository at this point in the history
In general, makes the dev dashboard tests faster and more consistent with other `build-system` tests.

1. **Removes AMP validation of output**. This was the slowest process in tests. It wasn't providing much value since we'd never fail validation anyway. We could potentially use Bento components later so might as well remove.

2. **Uses `ava`**. This is faster and can be listed along other `ava` tests instead of requiring a custom runner. Also removed special runner, task, and related configuration.

3. **Uses `posthtml` instead of `JSDOM`**. This reduces the elapsed time of remaining tests from `~1s` to `~200ms`.
  • Loading branch information
alanorozco authored Jun 16, 2021
1 parent fa14e5c commit 47bacd8
Show file tree
Hide file tree
Showing 13 changed files with 472 additions and 773 deletions.
1 change: 0 additions & 1 deletion amp.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ createTask('coverage-map', 'coverageMap');
createTask('css');
createTask('default', 'defaultTask', 'default-task');
createTask('dep-check', 'depCheck');
createTask('dev-dashboard-tests', 'devDashboardTests');
createTask('dist');
createTask('e2e');
createTask('firebase');
Expand Down
10 changes: 0 additions & 10 deletions build-system/pr-check/build-targets.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,16 +173,6 @@ const targetMatchers = {
file == 'build-system/global-configs/caches.json'
);
},
[Targets.DEV_DASHBOARD]: (file) => {
if (isOwnersFile(file)) {
return false;
}
return (
file == 'build-system/tasks/dev-dashboard-tests.js' ||
file == 'build-system/server/app.js' ||
file.startsWith('build-system/server/app-index/')
);
},
[Targets.DOCS]: (file) => {
if (isOwnersFile(file)) {
return false;
Expand Down
5 changes: 0 additions & 5 deletions build-system/pr-check/checks.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ function pushBuildWorkflow() {
timedExecOrDie('amp check-build-system');
timedExecOrDie('amp babel-plugin-tests');
timedExecOrDie('amp caches-json');
timedExecOrDie('amp dev-dashboard-tests');
timedExecOrDie('amp check-exact-versions');
timedExecOrDie('amp check-renovate-config');
timedExecOrDie('amp server-tests');
Expand Down Expand Up @@ -105,10 +104,6 @@ async function prBuildWorkflow() {
timedExecOrDie('amp markdown-toc');
}

if (buildTargetsInclude(Targets.DEV_DASHBOARD)) {
timedExecOrDie('amp dev-dashboard-tests');
}

if (buildTargetsInclude(Targets.OWNERS)) {
timedExecOrDie('amp check-owners --local_changes'); // only for PR builds
}
Expand Down
67 changes: 34 additions & 33 deletions build-system/server/app-index/test/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,44 +14,45 @@
* limitations under the License.
*/

const {expect} = require('chai');
const {JSDOM} = require('jsdom');
const posthtml = require('posthtml');

const parseHtmlChunk = (htmlStr) => {
const {body} = new JSDOM(htmlStr).window.document;
expect(body.children).to.have.length(1);
return body.firstElementChild;
};
function getElementChildren(content) {
return content?.filter?.((node) => typeof node !== 'string') || [];
}

const boundAttrRe = (attr) =>
new RegExp(`\\[${attr}\\]=(("[^"]+")|('[^']+')|([^\\s\\>]+))`);
async function posthtmlGetTextContent(document, matcher) {
let textContent;

// JSDom doesn't parse attributes whose names don't follow the spec, so
// our only way to test [attr] values is via regex.
const getBoundAttr = ({outerHTML}, attr) => {
const match = outerHTML.match(boundAttrRe(attr));
if (!match) {
return;
}
const [, valuePart] = match;
if (valuePart.charAt(0) == '"' || valuePart.charAt(0) == "'") {
return valuePart.substring(1, valuePart.length - 1);
}
return valuePart;
};
await posthtml([
(tree) => {
tree.match(matcher, (node) => {
if (node.content) {
textContent = node.content.join('');
}
return node;
});
return tree;
},
]).process(document);

const expectValidAmphtml = (validator, string) => {
const {errors: errorsAndWarnings, status} = validator.validateString(string);
const errors = errorsAndWarnings.filter(({severity}) => severity == 'ERROR');
return textContent;
}

// Compare with empty array instead of checking `to.be.empty` so
// validation errors are output as AssertionErrors.
expect(errors).to.deep.equal([]);
expect(status).to.equal('PASS');
};
async function extractMatching(document, matcher) {
const {html} = await posthtml([
(tree) => {
let matching = '';
tree.match(matcher, (node) => {
matching = node;
});
return [matching];
},
]).process(document);
return html.trim() || null;
}

module.exports = {
expectValidAmphtml,
getBoundAttr,
parseHtmlChunk,
extractMatching,
getElementChildren,
posthtmlGetTextContent,
};
Loading

0 comments on commit 47bacd8

Please sign in to comment.