From 438dcc6aa072f8b2f1f936ede6304e3979c2f7d4 Mon Sep 17 00:00:00 2001 From: Jaimin Panchal Date: Fri, 29 Sep 2017 11:56:20 -0400 Subject: [PATCH 1/2] Auction moduel refactor --- src/auction.js | 24 ++++++------- src/auctionManager.js | 14 +++----- src/prebid.js | 12 +++---- test/spec/auctionmanager_spec.js | 38 ++++++++++---------- test/spec/unit/pbjs_api_spec.js | 60 +++++++++++++------------------- 5 files changed, 64 insertions(+), 84 deletions(-) diff --git a/src/auction.js b/src/auction.js index 9a782b71549..9e00501abb9 100644 --- a/src/auction.js +++ b/src/auction.js @@ -82,7 +82,7 @@ events.on(CONSTANTS.EVENTS.BID_ADJUSTMENT, function (bid) { * * @returns {Auction} auction instance */ -function newAuction({adUnits, adUnitCodes}) { +export function newAuction({adUnits, adUnitCodes, callback, cbTimeout}) { let _adUnits = adUnits; let _adUnitCodes = adUnitCodes; let _bidderRequests = []; @@ -90,17 +90,17 @@ function newAuction({adUnits, adUnitCodes}) { let _auctionStart; let _auctionId = utils.getUniqueIdentifierStr(); let _auctionStatus; - let _callback; + let _callback = callback; let _timer; + let _timeout = cbTimeout; function addBidRequests(bidderRequests) { _bidderRequests = _bidderRequests.concat(bidderRequests) }; function addBidReceived(bidsReceived) { _bidsReceived = _bidsReceived.concat(bidsReceived); } - function startAuctionTimer(callback, cbtimeout) { - _callback = callback; + function startAuctionTimer() { const timedOut = true; const timeoutCallback = executeCallback.bind(null, timedOut); - let timer = setTimeout(timeoutCallback, cbtimeout); + let timer = setTimeout(timeoutCallback, _timeout); _timer = timer; } @@ -301,7 +301,7 @@ function newAuction({adUnits, adUnitCodes}) { } function doCallbacksIfNeeded() { - if (bid.timeToRespond > $$PREBID_GLOBAL$$.cbTimeout + $$PREBID_GLOBAL$$.timeoutBuffer) { + if (bid.timeToRespond > _timeout + $$PREBID_GLOBAL$$.timeoutBuffer) { executeCallback(true); } } @@ -334,18 +334,19 @@ function newAuction({adUnits, adUnitCodes}) { } } - function callBids(cbTimeout) { + function callBids() { + startAuctionTimer(); _auctionStatus = AUCTION_STARTED; _auctionStart = Date.now(); const auctionInit = { timestamp: _auctionStart, auctionId: _auctionId, - timeout: cbTimeout + timeout: _timeout }; events.emit(CONSTANTS.EVENTS.AUCTION_INIT, auctionInit); - let bidRequests = adaptermanager.makeBidRequests(_adUnits, _auctionStart, _auctionId, cbTimeout); + let bidRequests = adaptermanager.makeBidRequests(_adUnits, _auctionStart, _auctionId, _timeout); utils.logInfo(`Bids Requested for Auction with id: ${_auctionId}`, bidRequests); bidRequests.forEach(bidRequest => { addBidRequests(bidRequest); @@ -361,7 +362,6 @@ function newAuction({adUnits, adUnitCodes}) { getAdUnitCodes: () => _adUnitCodes, getBidRequests: () => _bidderRequests, getBidsReceived: () => _bidsReceived, - startAuctionTimer, callBids } } @@ -513,7 +513,3 @@ function groupByPlacement(bidsByPlacement, bid) { bidsByPlacement[bid.adUnitCode].bids.push(bid); return bidsByPlacement; } - -export function createAuction({adUnits, adUnitCodes}) { - return newAuction({adUnits, adUnitCodes}); -} diff --git a/src/auctionManager.js b/src/auctionManager.js index 691d7e5780b..5ca415d29c9 100644 --- a/src/auctionManager.js +++ b/src/auctionManager.js @@ -17,7 +17,7 @@ */ import { uniques, flatten } from './utils'; -import { createAuction, getStandardBidderSettings, AUCTION_COMPLETED } from 'src/auction'; +import { newAuction, getStandardBidderSettings, AUCTION_COMPLETED } from 'src/auction'; const CONSTANTS = require('./constants.json'); @@ -60,8 +60,10 @@ export function newAuctionManager() { .filter(uniques); }; - _public.createAuction = function({ adUnits, adUnitCodes }) { - return _createAuction({ adUnits, adUnitCodes }); + _public.createAuction = function({ adUnits, adUnitCodes, callback, cbTimeout }) { + const auction = newAuction({ adUnits, adUnitCodes, callback, cbTimeout }) + _addAuction(auction); + return auction; }; _public.findBidByAdId = function(adId) { @@ -74,12 +76,6 @@ export function newAuctionManager() { return getStandardBidderSettings()[CONSTANTS.JSON_MAPPING.ADSERVER_TARGETING]; }; - function _createAuction({ adUnits, adUnitCodes }) { - const auction = createAuction({ adUnits, adUnitCodes }) - _addAuction(auction); - return auction; - } - function _addAuction(auction) { _auctions.push(auction); } diff --git a/src/prebid.js b/src/prebid.js index 6e0489f324f..dd816a25b77 100644 --- a/src/prebid.js +++ b/src/prebid.js @@ -308,7 +308,7 @@ $$PREBID_GLOBAL$$.removeAdUnit = function (adUnitCode) { */ $$PREBID_GLOBAL$$.requestBids = function ({ bidsBackHandler, timeout, adUnits, adUnitCodes } = {}) { events.emit('requestBids'); - const cbTimeout = $$PREBID_GLOBAL$$.cbTimeout = timeout || config.getConfig('bidderTimeout'); + const cbTimeout = timeout || config.getConfig('bidderTimeout'); adUnits = adUnits || $$PREBID_GLOBAL$$.adUnits; utils.logInfo('Invoking $$PREBID_GLOBAL$$.requestBids', arguments); @@ -354,11 +354,11 @@ $$PREBID_GLOBAL$$.requestBids = function ({ bidsBackHandler, timeout, adUnits, a return; } - const auction = auctionManager.createAuction({adUnits, adUnitCodes}); - if (typeof bidsBackHandler === 'function') { - auction.startAuctionTimer(bidsBackHandler, cbTimeout); - } - auction.callBids(cbTimeout); + const auction = auctionManager.createAuction({adUnits, adUnitCodes, callback: bidsBackHandler, cbTimeout}); + // if (typeof bidsBackHandler === 'function') { + // auction.startAuctionTimer(bidsBackHandler, cbTimeout); + // } + auction.callBids(); }; /** diff --git a/test/spec/auctionmanager_spec.js b/test/spec/auctionmanager_spec.js index f492298d718..f2875dc18ad 100644 --- a/test/spec/auctionmanager_spec.js +++ b/test/spec/auctionmanager_spec.js @@ -470,8 +470,8 @@ describe('auctionmanager.js', function () { ] }]; adUnitCodes = ['adUnit-code']; - auction = auctionModule.createAuction({adUnits, adUnitCodes}); - createAuctionStub = sinon.stub(auctionModule, 'createAuction'); + auction = auctionModule.newAuction({adUnits, adUnitCodes, callback: function() {}, cbTimeout: 3000}); + createAuctionStub = sinon.stub(auctionModule, 'newAuction'); createAuctionStub.returns(auction); spec = { @@ -484,7 +484,7 @@ describe('auctionmanager.js', function () { }); afterEach(() => { - auctionModule.createAuction.restore(); + auctionModule.newAuction.restore(); }); it('should return proper price bucket increments for dense mode when cpm is in range 0-3', () => { @@ -493,7 +493,7 @@ describe('auctionmanager.js', function () { spec.buildRequests.returns([{'id': 123, 'method': 'POST'}]); spec.isBidRequestValid.returns(true); spec.interpretResponse.returns(bids); - auction.callBids(3000); + auction.callBids(); let registeredBid = auction.getBidsReceived().pop(); assert.equal(registeredBid.pbDg, '1.99', '0 - 3 hits at to 1 cent increment'); }); @@ -504,7 +504,7 @@ describe('auctionmanager.js', function () { spec.buildRequests.returns([{'id': 123, 'method': 'POST'}]); spec.isBidRequestValid.returns(true); spec.interpretResponse.returns(bids); - auction.callBids(3000); + auction.callBids(); let registeredBid = auction.getBidsReceived().pop(); assert.equal(registeredBid.pbDg, '4.35', '3 - 8 hits at 5 cent increment'); }); @@ -515,7 +515,7 @@ describe('auctionmanager.js', function () { spec.buildRequests.returns([{'id': 123, 'method': 'POST'}]); spec.isBidRequestValid.returns(true); spec.interpretResponse.returns(bids); - auction.callBids(3000); + auction.callBids(); let registeredBid = auction.getBidsReceived().pop(); assert.equal(registeredBid.pbDg, '19.50', '8 - 20 hits at 50 cent increment'); }); @@ -526,7 +526,7 @@ describe('auctionmanager.js', function () { spec.buildRequests.returns([{'id': 123, 'method': 'POST'}]); spec.isBidRequestValid.returns(true); spec.interpretResponse.returns(bids); - auction.callBids(3000); + auction.callBids(); let registeredBid = auction.getBidsReceived().pop(); assert.equal(registeredBid.pbDg, '20.00', '20+ caps at 20.00'); }); @@ -537,7 +537,7 @@ describe('auctionmanager.js', function () { spec.buildRequests.returns([{'id': 123, 'method': 'POST'}]); spec.isBidRequestValid.returns(true); spec.interpretResponse.returns(bids); - auction.callBids(3000); + auction.callBids(); let registeredBid = auction.getBidsReceived().pop(); assert.equal(registeredBid.adserverTargeting[`hb_deal`], 'test deal', 'dealId placed in adserverTargeting'); }); @@ -549,7 +549,7 @@ describe('auctionmanager.js', function () { spec.buildRequests.returns([{'id': 123, 'method': 'POST'}]); spec.isBidRequestValid.returns(true); spec.interpretResponse.returns(bids); - auction.callBids(3000); + auction.callBids(); let registeredBid = auction.getBidsReceived().pop(); assert.equal(registeredBid.adserverTargeting.hb_bidder, 'sampleBidder'); assert.equal(registeredBid.adserverTargeting.extra, 'stuff'); @@ -578,7 +578,7 @@ describe('auctionmanager.js', function () { spec.buildRequests.returns([{'id': 123, 'method': 'POST'}]); spec.isBidRequestValid.returns(true); spec.interpretResponse.returns(bids1); - auction.callBids(3000); + auction.callBids(); assert.equal(bidsRecCount + 1, auction.getBidsReceived().length); utils.getBidRequest.restore(); @@ -608,7 +608,7 @@ describe('auctionmanager.js', function () { spec.buildRequests.returns([{'id': 123, 'method': 'POST'}]); spec.isBidRequestValid.returns(true); spec.interpretResponse.returns(bids1); - auction.callBids(3000); + auction.callBids(); assert.equal(bidsRecCount, auction.getBidsReceived().length); utils.getBidRequest.restore(); @@ -640,7 +640,7 @@ describe('auctionmanager.js', function () { spec.buildRequests.returns([{'id': 123, 'method': 'POST'}]); spec.isBidRequestValid.returns(true); spec.interpretResponse.returns(bids1); - auction.callBids(3000); + auction.callBids(); assert.equal(bidsRecCount + 1, auction.getBidsReceived().length); utils.getBidRequest.restore(); @@ -685,7 +685,7 @@ describe('auctionmanager.js', function () { spec.buildRequests.returns([{'id': 123, 'method': 'POST'}]); spec.isBidRequestValid.returns(true); spec.interpretResponse.returns(bids1); - auction.callBids(3000); + auction.callBids(); const addedBid = auction.getBidsReceived().pop(); assert.equal(addedBid.renderer.url, 'renderer.js'); }); @@ -729,7 +729,7 @@ describe('auctionmanager.js', function () { spec.buildRequests.returns([{'id': 123, 'method': 'POST'}]); spec.isBidRequestValid.returns(true); spec.interpretResponse.returns(bids1); - auction.callBids(3000); + auction.callBids(); const addedBid = auction.getBidsReceived().pop(); assert.equal(addedBid.width, 300); @@ -836,8 +836,8 @@ describe('auctionmanager.js', function () { ] }]; adUnitCodes = ['adUnit-code', 'adUnit-code-1']; - auction = auctionModule.createAuction({adUnits, adUnitCodes}); - createAuctionStub = sinon.stub(auctionModule, 'createAuction'); + auction = auctionModule.newAuction({adUnits, adUnitCodes, callback: function() {}, cbTimeout: 3000}); + createAuctionStub = sinon.stub(auctionModule, 'newAuction'); createAuctionStub.returns(auction); spec = { @@ -858,7 +858,7 @@ describe('auctionmanager.js', function () { }); afterEach(() => { - auctionModule.createAuction.restore(); + auctionModule.newAuction.restore(); }); it('should not alter bid adID', () => { @@ -873,7 +873,7 @@ describe('auctionmanager.js', function () { spec1.isBidRequestValid.returns(true); spec1.interpretResponse.returns(bids1); - auction.callBids(3000); + auction.callBids(); const addedBid2 = auction.getBidsReceived().pop(); assert.equal(addedBid2.adId, bids1[0].requestId); @@ -896,7 +896,7 @@ describe('auctionmanager.js', function () { spec1.isBidRequestValid.returns(true); spec1.interpretResponse.returns(bids1); - auction.callBids(3000); + auction.callBids(); let length = auction.getBidsReceived().length; const addedBid2 = auction.getBidsReceived().pop(); diff --git a/test/spec/unit/pbjs_api_spec.js b/test/spec/unit/pbjs_api_spec.js index d8c90d92ba8..79446d645e9 100644 --- a/test/spec/unit/pbjs_api_spec.js +++ b/test/spec/unit/pbjs_api_spec.js @@ -36,7 +36,9 @@ var config = require('test/fixtures/config.json'); $$PREBID_GLOBAL$$ = $$PREBID_GLOBAL$$ || {}; var adUnits = getAdUnits(); var adUnitCodes = getAdUnits().map(unit => unit.code); -var auction = auctionManager.createAuction({adUnits, adUnitCodes}); +var bidsBackHandler = function() {}; +const timeout = 2000; +var auction = auctionManager.createAuction({adUnits, adUnitCodes, callback: bidsBackHandler, cbTimeout: timeout}); auction.getBidRequests = getBidRequests; auction.getBidsReceived = getBidResponses; auction.getAdUnits = getAdUnits; @@ -812,6 +814,7 @@ describe('Unit: Prebid Module', function () { describe('requestBids', () => { var adUnitsBackup; var auctionManagerStub; + let logMessageSpy describe('part 1', () => { beforeEach(() => { @@ -819,54 +822,39 @@ describe('Unit: Prebid Module', function () { auctionManagerStub = sinon.stub(auctionManager, 'createAuction', function() { return auction; }); + logMessageSpy = sinon.spy(utils, 'logMessage'); }); afterEach(() => { auction.getAdUnits = adUnitsBackup; auctionManager.createAuction.restore(); + utils.logMessage.restore(); resetAuction(); }); - it('should add bidsBackHandler callback to auction instance', () => { - var spyAuctionCallBack = sinon.spy(auction, 'startAuctionTimer'); - var requestObj = { - bidsBackHandler: function bidsBackHandlerCallback() {}, - adUnits: auction.getAdUnits() - }; - $$PREBID_GLOBAL$$.requestBids(requestObj); - const spyArgs = auction.startAuctionTimer.getCall(0); - expect(spyArgs['args'][0].toString()).to.equal(requestObj.bidsBackHandler.toString()); - auction.startAuctionTimer.restore(); - }); - it('should log message when adUnits not configured', () => { - const logMessageSpy = sinon.spy(utils, 'logMessage'); - $$PREBID_GLOBAL$$.adUnits = []; $$PREBID_GLOBAL$$.requestBids({}); assert.ok(logMessageSpy.calledWith('No adUnits configured. No bids requested.'), 'expected message was logged'); - utils.logMessage.restore(); }); it('should execute callback after timeout', () => { - var logMessageSpy = sinon.spy(utils, 'logMessage'); - var clock = sinon.useFakeTimers(); - var requestObj = { + let clock = sinon.useFakeTimers(); + let requestObj = { bidsBackHandler: function bidsBackHandlerCallback() {}, - timeout: 2000, + timeout: timeout, adUnits: auction.getAdUnits() }; $$PREBID_GLOBAL$$.requestBids(requestObj); - var re = new RegExp('^Auction [0-9A-Za-z]+ timedOut$'); + let re = new RegExp('^Auction [0-9A-Za-z]+ timedOut$'); clock.tick(requestObj.timeout - 1); assert.ok(logMessageSpy.neverCalledWith(sinon.match(re)), 'executeCallback not called'); clock.tick(1); assert.ok(logMessageSpy.calledWith(sinon.match(re)), 'executeCallback called'); - utils.logMessage.restore(); clock.restore(); }); @@ -914,14 +902,14 @@ describe('Unit: Prebid Module', function () { ] }]; adUnitCodes = ['adUnit-code']; - let auction = auctionModule.createAuction({adUnits, adUnitCodes}); + let auction = auctionModule.newAuction({adUnits, adUnitCodes, callback: function() {}, cbTimeout: timeout}); spyCallBids = sinon.spy(adaptermanager, 'callBids'); - createAuctionStub = sinon.stub(auctionModule, 'createAuction'); + createAuctionStub = sinon.stub(auctionModule, 'newAuction'); createAuctionStub.returns(auction); }) after(() => { - auctionModule.createAuction.restore(); + auctionModule.newAuction.restore(); adaptermanager.callBids.restore(); }); @@ -948,14 +936,14 @@ describe('Unit: Prebid Module', function () { ] }]; adUnitCodes = ['adUnit-code']; - let auction = auctionModule.createAuction({adUnits, adUnitCodes}); + let auction = auctionModule.newAuction({adUnits, adUnitCodes, callback: function() {}, cbTimeout: timeout}); spyCallBids = sinon.spy(adaptermanager, 'callBids'); - createAuctionStub = sinon.stub(auctionModule, 'createAuction'); + createAuctionStub = sinon.stub(auctionModule, 'newAuction'); createAuctionStub.returns(auction); }) after(() => { - auctionModule.createAuction.restore(); + auctionModule.newAuction.restore(); adaptermanager.callBids.restore(); }); @@ -983,14 +971,14 @@ describe('Unit: Prebid Module', function () { ] }]; adUnitCodes = ['adUnit-code']; - let auction = auctionModule.createAuction({adUnits, adUnitCodes}); + let auction = auctionModule.newAuction({adUnits, adUnitCodes, callback: function() {}, cbTimeout: timeout}); spyCallBids = sinon.spy(adaptermanager, 'callBids'); - createAuctionStub = sinon.stub(auctionModule, 'createAuction'); + createAuctionStub = sinon.stub(auctionModule, 'newAuction'); createAuctionStub.returns(auction); }) after(() => { - auctionModule.createAuction.restore(); + auctionModule.newAuction.restore(); adaptermanager.callBids.restore(); }); @@ -1018,11 +1006,11 @@ describe('Unit: Prebid Module', function () { ] }]; let adUnitCodes = ['adUnit-code']; - let auction = auctionModule.createAuction({adUnits, adUnitCodes}); + let auction = auctionModule.newAuction({adUnits, adUnitCodes, callback: function() {}, cbTimeout: timeout}); adUnits[0]['mediaType'] = 'native'; adUnitCodes = ['adUnit-code']; - let auction1 = auctionModule.createAuction({adUnits, adUnitCodes}); + let auction1 = auctionModule.newAuction({adUnits, adUnitCodes, callback: function() {}, cbTimeout: timeout}); adUnits = [{ code: 'adUnit-code', @@ -1031,16 +1019,16 @@ describe('Unit: Prebid Module', function () { {bidder: 'appnexusAst', params: {placementId: 'id'}} ] }]; - let auction3 = auctionModule.createAuction({adUnits, adUnitCodes}); + let auction3 = auctionModule.newAuction({adUnits, adUnitCodes, callback: function() {}, cbTimeout: timeout}); - let createAuctionStub = sinon.stub(auctionModule, 'createAuction'); + let createAuctionStub = sinon.stub(auctionModule, 'newAuction'); createAuctionStub.onCall(0).returns(auction1); createAuctionStub.onCall(2).returns(auction3); createAuctionStub.returns(auction); }); after(() => { - auctionModule.createAuction.restore(); + auctionModule.newAuction.restore(); }); beforeEach(() => { From f94069edfb15293d4eb76f1c28d55db1984e39e0 Mon Sep 17 00:00:00 2001 From: Jaimin Panchal Date: Fri, 29 Sep 2017 15:29:37 -0400 Subject: [PATCH 2/2] remove comment and global cbtimeout --- src/prebid.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/prebid.js b/src/prebid.js index dd816a25b77..760063bc1a1 100644 --- a/src/prebid.js +++ b/src/prebid.js @@ -42,9 +42,6 @@ $$PREBID_GLOBAL$$.bidderSettings = $$PREBID_GLOBAL$$.bidderSettings || {}; /** @deprecated - use pbjs.setConfig({ bidderTimeout: }) */ $$PREBID_GLOBAL$$.bidderTimeout = $$PREBID_GLOBAL$$.bidderTimeout; -// current timeout set in `requestBids` or to default `bidderTimeout` -$$PREBID_GLOBAL$$.cbTimeout = $$PREBID_GLOBAL$$.cbTimeout || 200; - // timeout buffer to adjust for bidder CDN latency $$PREBID_GLOBAL$$.timeoutBuffer = 200; @@ -355,9 +352,6 @@ $$PREBID_GLOBAL$$.requestBids = function ({ bidsBackHandler, timeout, adUnits, a } const auction = auctionManager.createAuction({adUnits, adUnitCodes, callback: bidsBackHandler, cbTimeout}); - // if (typeof bidsBackHandler === 'function') { - // auction.startAuctionTimer(bidsBackHandler, cbTimeout); - // } auction.callBids(); };