PBjs Core and Tests : fix spurious tests (#11767)

* Fix spurious tests * fix ajax tests
prebid · Jun 12, 2024 · 12e2e1c · 12e2e1c
1 parent df57b81
commit 12e2e1c
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 58 deletions.
diff --git 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/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,52 +159,48 @@ 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({
  adId: '123',
  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;
+ })
  });
  });
  });

diff --git 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']
- })
- });
  });
  });
  });