From 9feb3596b8544a4fb905fe3b2028e01dd91e98d8 Mon Sep 17 00:00:00 2001 From: Demetrio Girardi Date: Tue, 11 Jun 2024 11:36:27 -0700 Subject: [PATCH 1/2] Fix spurious tests --- .../spec/creative/crossDomainCreative_spec.js | 93 +++++++++++-------- 1 file changed, 53 insertions(+), 40 deletions(-) diff --git a/test/spec/creative/crossDomainCreative_spec.js b/test/spec/creative/crossDomainCreative_spec.js index f4c98aa7b50..1bba54c752d 100644 --- a/test/spec/creative/crossDomainCreative_spec.js +++ b/test/spec/creative/crossDomainCreative_spec.js @@ -38,6 +38,27 @@ describe('cross-domain creative', () => { renderAd = renderer(win); }) + function waitFor(predicate, timeout = 1000) { + let timedOut = false; + return new Promise((resolve, reject) => { + let to = setTimeout(() => { + timedOut = true; + reject(new Error('timeout')) + }, timeout) + resolve = (orig => () => { clearTimeout(to); orig() })(resolve); + function check() { + if (!timedOut) { + setTimeout(() => { + if (predicate()) { + resolve() + } else check(); + }, 50) + } + } + check(); + }) + } + it('derives postMessage target origin from pubUrl ', () => { renderAd({pubUrl: 'https://domain.com:123/path'}); expect(messages[0].targetOrigin).to.eql('https://domain.com:123') @@ -75,11 +96,11 @@ describe('cross-domain creative', () => { sinon.assert.notCalled(mkIframe); }) - it('signals AD_RENDER_FAILED on exceptions', (done) => { + it('signals AD_RENDER_FAILED on exceptions', () => { mkIframe.callsFake(() => { throw new Error('error message') }); renderAd({adId: '123', pubUrl: ORIGIN}); reply({message: MESSAGE_RESPONSE, adId: '123', ad: 'markup'}); - setTimeout(() => { + return waitFor(() => messages[1]?.payload).then(() => { expect(messages[1].payload).to.eql({ message: MESSAGE_EVENT, adId: '123', @@ -89,8 +110,7 @@ describe('cross-domain creative', () => { message: 'error message' } }) - done(); - }, 100) + }) }); describe('renderer', () => { @@ -99,7 +119,7 @@ describe('cross-domain creative', () => { win.document.body.appendChild.callsFake(document.body.appendChild.bind(document.body)); }); - it('sets up and runs renderer', (done) => { + it('sets up and runs renderer', () => { window._render = sinon.stub(); const data = { message: MESSAGE_RESPONSE, @@ -108,14 +128,11 @@ describe('cross-domain creative', () => { } renderAd({adId: '123', pubUrl: ORIGIN}); reply(data); - setTimeout(() => { - try { - sinon.assert.calledWith(window._render, data, sinon.match.any, win); - done() - } finally { - delete window._render; - } - }, 100) + return waitFor(() => window._render.args.length).then(() => { + sinon.assert.calledWith(window._render, data, sinon.match.any, win); + }).finally(() => { + delete window._render; + }) }); Object.entries({ @@ -125,14 +142,14 @@ describe('cross-domain creative', () => { 'rejects (w/error)': ['window.render = function() { return Promise.reject(new Error("msg")) }'], 'rejects (w/reason)': ['window.render = function() { return Promise.reject({reason: "other", message: "msg"}) }', 'other'], }).forEach(([t, [renderer, reason = ERROR_EXCEPTION, message = 'msg']]) => { - it(`signals AD_RENDER_FAILED on renderer that ${t}`, (done) => { + it(`signals AD_RENDER_FAILED on renderer that ${t}`, () => { renderAd({adId: '123', pubUrl: ORIGIN}); reply({ message: MESSAGE_RESPONSE, adId: '123', renderer }); - setTimeout(() => { + return waitFor(() => messages[1]?.payload).then(() => { sinon.assert.match(messages[1].payload, { adId: '123', message: MESSAGE_EVENT, @@ -142,34 +159,33 @@ describe('cross-domain creative', () => { message: sinon.match(val => message == null || message === val) } }); - done(); - }, 100) + }) }) }); - it('signals AD_RENDER_SUCCEEDED when renderer resolves', (done) => { + it('signals AD_RENDER_SUCCEEDED when renderer resolves', () => { renderAd({adId: '123', pubUrl: ORIGIN}); reply({ message: MESSAGE_RESPONSE, adId: '123', renderer: 'window.render = function() { return new Promise((resolve) => { window.parent._resolve = resolve })}' }); - setTimeout(() => { + return waitFor(() => window._resolve).then(() => { expect(messages[1]).to.not.exist; window._resolve(); - setTimeout(() => { - sinon.assert.match(messages[1].payload, { - adId: '123', - message: MESSAGE_EVENT, - event: EVENT_AD_RENDER_SUCCEEDED - }) - delete window._resolve; - done(); - }, 100) - }, 100) + return waitFor(() => messages[1]?.payload) + }).then(() => { + sinon.assert.match(messages[1].payload, { + adId: '123', + message: MESSAGE_EVENT, + event: EVENT_AD_RENDER_SUCCEEDED + }) + }).finally(() => { + delete window._resolve; + }) }) - it('is provided a sendMessage that accepts replies', (done) => { + it('is provided a sendMessage that accepts replies', () => { renderAd({adId: '123', pubUrl: ORIGIN}); window._reply = sinon.stub(); reply({ @@ -177,17 +193,14 @@ describe('cross-domain creative', () => { message: MESSAGE_RESPONSE, renderer: 'window.render = function(_, {sendMessage}) { sendMessage("test", "data", function(reply) { window.parent._reply(reply) }) }' }); - setTimeout(() => { + return waitFor(() => messages[1]?.payload).then(() => { reply('response', 1); - setTimeout(() => { - try { - sinon.assert.calledWith(window._reply, sinon.match({data: JSON.stringify('response')})); - done(); - } finally { - delete window._reply; - } - }, 100) - }, 100) + return waitFor(() => window._reply.args.length) + }).then(() => { + sinon.assert.calledWith(window._reply, sinon.match({data: JSON.stringify('response')})); + }).finally(() => { + delete window._reply; + }) }); }); }); From 4933817e51e55b5f7402a690049f0738d9914b21 Mon Sep 17 00:00:00 2001 From: Demetrio Girardi Date: Tue, 11 Jun 2024 12:09:37 -0700 Subject: [PATCH 2/2] fix ajax tests --- src/ajax.js | 2 +- test/spec/unit/core/ajax_spec.js | 32 +++++++++++++++----------------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/src/ajax.js b/src/ajax.js index 92bff6dd527..1ef100e7fd3 100644 --- a/src/ajax.js +++ b/src/ajax.js @@ -130,7 +130,7 @@ export function attachCallbacks(fetchPm, callback) { success: typeof callback === 'function' ? callback : () => null, error: (e, x) => logError('Network error', e, x) }; - fetchPm.then(response => response.text().then((responseText) => [response, responseText])) + return fetchPm.then(response => response.text().then((responseText) => [response, responseText])) .then(([response, responseText]) => { const xhr = toXHR(response, responseText); response.ok || response.status === 304 ? success(responseText, xhr) : error(response.statusText, xhr); diff --git a/test/spec/unit/core/ajax_spec.js b/test/spec/unit/core/ajax_spec.js index dd03ad1a761..8140123d9fc 100644 --- a/test/spec/unit/core/ajax_spec.js +++ b/test/spec/unit/core/ajax_spec.js @@ -405,24 +405,22 @@ describe('attachCallbacks', () => { }).forEach(([cbType, makeResponse]) => { it(`do not choke ${cbType} callbacks`, () => { const {response} = makeResponse(); - return new Promise((resolve) => { - const result = {success: false, error: false}; - attachCallbacks(Promise.resolve(response), { - success() { - result.success = true; - throw new Error(); - }, - error() { - result.error = true; - throw new Error(); - } + const result = {success: false, error: false}; + return attachCallbacks(Promise.resolve(response), { + success() { + result.success = true; + throw new Error(); + }, + error() { + result.error = true; + throw new Error(); + } + }).catch(() => null) + .then(() => { + Object.entries(result).forEach(([typ, ran]) => { + expect(ran).to.be[typ === cbType ? 'true' : 'false']; + }); }); - setTimeout(() => resolve(result), 20); - }).then(result => { - Object.entries(result).forEach(([typ, ran]) => { - expect(ran).to.be[typ === cbType ? 'true' : 'false'] - }) - }); }); }); });