Refactor 3p ad unit test to remove dependency on amp-ad.js. #5417
Conversation
| return getAd().then(ad => { | ||
| const src = ad.firstChild.getAttribute('src'); | ||
|
|
||
| return ad3p.layoutCallback().then(() => { |
There was a problem hiding this comment.
already tested before. Remove either one.
There was a problem hiding this comment.
removed from previous test
| impl.onLayoutMeasure(); | ||
| return impl.layoutCallback().then(() => { | ||
| const src = ampAd.firstChild.getAttribute('src'); | ||
| it('should allow position:fixed with whitelisted ad container', () => { |
There was a problem hiding this comment.
also mention in description. pass container name to iframe
| clock.tick(100); | ||
| expect(impl.renderOutsideViewport()).to.equal(1.25); | ||
| }); | ||
| it('should allow position:fixed with whitelisted ad container', () => { |
There was a problem hiding this comment.
forgot to change the test name of the other test
|
|
||
| beforeEach(() => { | ||
| sandbox = sinon.sandbox.create(); | ||
| return createIframePromise().then(iframe => { |
There was a problem hiding this comment.
I see comment //Make sure we run tests without CID available by default from our tests before. I don't feel this is necessary, but just to confirm we don't need to check this anymore.
There was a problem hiding this comment.
I don't think this is valid anymore. We should just have separate test cases for w/ and w/o CID
| href: 'blah', | ||
| describe('renderOutsideViewport', () => { | ||
| it('should allow rendering within 3 viewports by default', () => { | ||
| expect(ad3p.renderOutsideViewport()).to.equal(3); |
There was a problem hiding this comment.
wait, this work? I find the test statement totally different from what we have before. We don't have to wait for a sec now?
There was a problem hiding this comment.
oh, i think i get it, because we never layout ad in this case. But then should we test that renderOutsideViewport will be false for 1 sec after we layout an ad?
There was a problem hiding this comment.
the 1 sec logic is in cocurrent-load.js, it should be better tested there
There was a problem hiding this comment.
but we don't have test there 😢
| it('should prefer-viewability-over-views', () => { | ||
| ad3p.element.setAttribute( | ||
| 'data-loading-strategy', 'prefer-viewability-over-views'); | ||
| expect(ad3p.renderOutsideViewport()).to.equal(1.25); |
| return iframe.addElement(a); | ||
| }); | ||
| } | ||
| // TODO: add test for noContentHandler_() |
There was a problem hiding this comment.
The code for noContentHandler_ may be moved outside 3p-ad-impl later. But before that let's keep still keep the test.
There was a problem hiding this comment.
The test will be very different from the current one. I would rather rewrite the test once the code here got extracted to somewhere else.
There was a problem hiding this comment.
But then we wouldn't have test coverage for noContentHandler. I am not sure about this because we are going to change that function, and we definitely want to have test for the upcoming change.
|
lgtm |
No description provided.