Amp-Img Bind Integration Tests #7721
Conversation
|
@choumx PTAL |
| @@ -53,10 +52,14 @@ describe.configure().retryOnSaucelabs().run('integration amp-bind', function() { | |||
| }); | |||
| } | |||
|
|
|||
| function setButtonBinding(button, binding) { | |||
| button.setAttribute('on', `tap:AMP.setState(${binding})`); | |||
There was a problem hiding this comment.
Bit of a misnomer. A "binding" is a (property, expression) tuple on an expression -- the element's property is "bound" to the expression. Updating a state var via AMP.setState is not a binding.
I'd rename this method function setStateOnTap(button, key, value).
There was a problem hiding this comment.
function deleted based on other comments
| @@ -66,9 +69,10 @@ describe.configure().retryOnSaucelabs().run('integration amp-bind', function() { | |||
|
|
|||
| it('should update CSS class when class binding changes', () => { | |||
| const textElement = fixture.doc.getElementById('textElement'); | |||
| const button = fixture.doc.getElementById('mutateTextButton'); | |||
| const changeClassButton = | |||
There was a problem hiding this comment.
Nit: When naming, be only as specific as necessary to disambiguate. I.e. in the absence of other buttons in this scope, just button would suffice. Otherwise readers have to take some time to understand what the "changeClass" prefix signifies.
| fixture.doc.getElementById('changeImgSrcButton'); | ||
| const img = fixture.doc.getElementById('image'); | ||
| expect(img.getAttribute('src')).to.equal('https://lh3.googleusercontent' + | ||
| '.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w300-h200-no'); |
There was a problem hiding this comment.
Is there a more pithy image URL you could use here? Does it matter than if it's valid?
| const originalSrc = 'https://lh3.googleusercontent.com/' + | ||
| '5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w300-h200-no'; | ||
| expect(img.getAttribute('src')).to.equal(originalSrc); | ||
| const newSrc = '__amp_source_origin'; |
There was a problem hiding this comment.
Prefer inlined literals over local vars in unit tests. IMO duplicating simple literals is more readable.
| expect(img.getAttribute('src')).to.equal(originalSrc); | ||
| const newSrc = '__amp_source_origin'; | ||
| const blockedURLBinding = `imageSrc="${newSrc}"`; | ||
| setButtonBinding(changeImgSrcButton, blockedURLBinding); |
There was a problem hiding this comment.
Actually probably more readable if you just used a different button for this.
|
@choumx let me know if you have any other comments |
Added integration tests for amp-img that test the src, alt, width, and height attributes. I did not add a test for srcset but I can if needed, though these tests will become quite large if we test every supported attribute on every supported amp element exhaustively.