✅♻️Refactor and test devdash#20651
Conversation
7d55371 to
9114135
Compare
|
@sparhami Addressed, PTAL. |
| // our only way to test [attr] values is via regex. | ||
| const getBoundAttr = (el, attr) => { | ||
| const match = el./*OK*/outerHTML.match(boundAttrRe(attr)); | ||
| if (match) { |
There was a problem hiding this comment.
would prefer to use a guard clause here and reduce nesting:
if (!match) {
return;
}|
|
||
| const expectValidAmphtml = (validator, string) => { | ||
| const {errors, status} = validator.validateString(string); | ||
| expect(errors).to.deep.equal([]); |
There was a problem hiding this comment.
One difference between using .to.be.empty is that the assertion output is different:
AssertionError: expected [ Array(12) ] to be empty
at expectValidAmphtml (build-system/app-index/test/helpers.js:51:23)
...
Whereas you get the actual comparison values when comparing to an empty array, so validation errors are output for free:
AssertionError: expected [ Array(12) ] to deeply equal []
+ expected - actual
-[
- {
- "category": "MANDATORY_AMP_TAG_MISSING_OR_INCORRECT"
- "code": "MANDATORY_ATTR_MISSING"
- "col": 0
- "line": 1
- "message": "The mandatory attribute '⚡' is missing in tag 'html'." [ - "params": [
- "⚡" [ - "html"
- ]
- "severity": "ERROR"
- "specUrl": "https://www.ampproject.org/docs/reference/spec#required-markup"
- }
...
There was a problem hiding this comment.
validateString returns the validationresult object, which has errors on it. note that errors will also include warnings. if just checking for validity then status would be better to check because if the given string only has warnings it's still valid and errors is not empty.
There was a problem hiding this comment.
Gotcha, thanks for weighing in @honeybadgerdontcare. Filtered out non-ERRORs so this won't fail on warning.
| <html> | ||
| <head></head> | ||
| <body> | ||
| <form action=whatever.com></form> |
| <head></head> | ||
| <body> | ||
| <div> | ||
| <template type=amp-mustache></template> |
| <div> | ||
| <amp-bar></amp-bar> | ||
| <div> | ||
| <amp-foo-bar-baz many=1 attributes=2> |
| </amp-foo-bar-baz> | ||
| </div> | ||
| <input> | ||
| <amp-state id=myState></amp-state> |
| <div> | ||
| <amp-bar></amp-bar> | ||
| <div> | ||
| <amp-foo-bar-baz many=1 attributes=2> |
| selectModePrefix: '/', | ||
| })); | ||
|
|
||
| const {length} = root.getElementsByTagName('amp-list'); |
There was a problem hiding this comment.
you can use .to.have.length on things that are array-like, so you could do
expect(root.getElementsByTagName('amp-list')).to.have.length(1);
|
@alanorozco Sorry for missing this! Was just about to review this. Thanks for the implementation! Thank you @sparhami for doing the lengthy review! 😄 |
| "dependencies": { | ||
| "@ampproject/animations": "0.1.1", | ||
| "@ampproject/worker-dom": "0.2.17", | ||
| "amphtml-validator": "^1.0.23", |
There was a problem hiding this comment.
This should be in devDependencies and pinned to a version (no ^).
- Restructure and extract `template.js` modules for semantic grouping. - Fold `html.js` and `html-helpers.js` into one. - Test exported HTML renderer outputs by DOM selection. - Test several helpers. - Test top-level renderer output to ensure all devdash pages are AMPHTML valid!
- Restructure and extract `template.js` modules for semantic grouping. - Fold `html.js` and `html-helpers.js` into one. - Test exported HTML renderer outputs by DOM selection. - Test several helpers. - Test top-level renderer output to ensure all devdash pages are AMPHTML valid!
template.jsmodules for semantic grouping.html.jsandhtml-helpers.jsinto one.