refactor 3p ad test #5193
refactor 3p ad test #5193
Conversation
| const fragment = url.substr(url.indexOf('#') + 1); | ||
| const data = JSON.parse(fragment); | ||
|
|
||
| expect(data.type).to.equal('_ping_'); |
There was a problem hiding this comment.
This will give better error message when it fails:
expect(data).have.property('type', '_ping_');
| 'http://ads.localhost:9876/dist.3p/current/integration.js'); | ||
| const preconnects = doc.querySelectorAll( | ||
| 'link[rel=preconnect]'); | ||
| expect(preconnects[preconnects.length - 1].href).to.equal( |
| it('should only layout once', () => { | ||
| return getAd().then(ad => { | ||
| ad.implementation_.unlayoutCallback(); | ||
| it('should only layout once', () => { |
There was a problem hiding this comment.
maybe move this test case up for better grouping.
| }); | ||
| ad.appendChild(fallback); | ||
| ad.appendChild(placeholder); | ||
| expect(placeholder.classList.contains('amp-hidden')).to.be.false; |
There was a problem hiding this comment.
expect(placeholder.classList).to.not.contain(...)
There was a problem hiding this comment.
I can do this here. because there will be error:
AssertionError: object tested must be an array, an object, or a string, but domtokenlist given
| const placeholderEl = ad.querySelector('[placeholder]'); | ||
|
|
||
| ad.implementation_.noContentHandler_(); | ||
| expect(ad).to.have.class('amp-notsupported'); |
There was a problem hiding this comment.
ha, does this work? if it is, we can apply it to line 192
There was a problem hiding this comment.
This only works after toggleFallback() is called. https://github.com/ampproject/amphtml/blob/master/src/custom-element.js#L1262
Can't apply to line 192 because we haven't #toggleFallback then.
There was a problem hiding this comment.
No, I was just wondering if the chai assert .to.have.class works.
There was a problem hiding this comment.
tested. worked. changed to use .to.have.class
|
|
||
| ad.implementation_.noContentHandler_(); | ||
| expect(ad).to.have.class('amp-notsupported'); | ||
| expect(placeholderEl.classList.contains('amp-hidden')).to.be.true; |
| clock.tick(900); | ||
| expect(impl.renderOutsideViewport()).to.be.false; | ||
| clock.tick(100); | ||
| expect(impl.renderOutsideViewport()).not.to.be.false; |
There was a problem hiding this comment.
is this same as to.be.true?
There was a problem hiding this comment.
it's not in this special case. #renderOutsideViewport() return {boolean|number}
| const placeholderEl = ad.querySelector('[placeholder]'); | ||
|
|
||
| ad.implementation_.noContentHandler_(); | ||
| expect(ad).to.have.class('amp-notsupported'); |
There was a problem hiding this comment.
No, I was just wondering if the chai assert .to.have.class works.
follow up with ad test refactoring. PTAL