Aliasbidder fix #1652
Aliasbidder fix #1652
Conversation
…-fix # Conflicts: # src/adaptermanager.js
src/adaptermanager.js
Outdated
| newAdapter.setBidderCode(alias); | ||
| } else { | ||
| let spec = Object.assign({}, bidAdaptor, { code: alias }); | ||
| newAdapter = newBidder(spec); |
There was a problem hiding this comment.
This is a little buggy.
The bidderFactory accepts a spec, but only returns an object like:
{
callBids: function() { ... }
}
So in this code, you're using objects of the shape:
newAdapter = newBidder({
code: alias,
callBids: function() { ... }
});
...but newBIdder needs a spec. The typedef doesn't have callBids, but does require several other things which you're not giving here (interpretResponse, buildRequests, etc).
So... if callBids gets called on an aliased bidder, there'll be all sorts of errors about missing functions.
| @@ -138,6 +138,9 @@ export function registerBidder(spec) { | |||
| */ | |||
| export function newBidder(spec) { | |||
| return Object.assign(new Adapter(spec.code), { | |||
| getSpec: function() { | |||
There was a problem hiding this comment.
It might be worth doing an Object.freeze on the spec here.
Otherwise someone could call getSpec() and then mutate that object, producing some absolutely horrible bugs. IMO, the burden lies on the file implementer to make sure there's no way to "misuse" the functions they make public.
test/spec/unit/pbjs_api_spec.js
Outdated
| const thisSpec = Object.assign(spec, { supportedMediaTypes: ['video'] }); | ||
| registerBidder(thisSpec); | ||
| const alias = 'aliasBidder'; | ||
| $$PREBID_GLOBAL$$.aliasBidder(CODE, alias); |
There was a problem hiding this comment.
Try to avoid $$PREBID_GLOBAL$$ in tests (see #1508).
Prebid-specifics aside... strong unit tests cover the smallest amount of code possible. All your changes are in adapterManager... so it's much better if your tests import the adapterManager and call the functions directly. The $$PREBID_GLOBAL$$.aliasBidder function is at a higher level, and would be better tested by stubbing the adapterManager with sinon and making sure the methods get called with the right args.
On a less important note... you could probably write these in test/spec/unit/adapterManager_spec.js. This file is much larger than it should be already.
| @@ -3,6 +3,9 @@ import AdapterManager from 'src/adaptermanager'; | |||
| import { getAdUnits } from 'test/fixtures/fixtures'; | |||
| import CONSTANTS from 'src/constants.json'; | |||
| import * as utils from 'src/utils'; | |||
| import { registerBidder } from 'src/adapters/bidderFactory'; | |||
|
|
|||
| require('modules/appnexusAstBidAdapter'); | |||
There was a problem hiding this comment.
Not accidental, i took a shortcut to test old way of aliasing adapter. Since we will be only keeping this code for a short time.
But anyway, updated test case to make it as it should be.
There was a problem hiding this comment.
ah ok... no worries either way.
….30.0 to aolgithub-master * commit '5a8d2bf93ee15071a78e24ac976103cacf3c6021': (35 commits) Added changelog entry. Prebid 0.30.1 Release Remove undefined variable usage (prebid#1662) fixes bug for IE when invalid value passed to parse (prebid#1657) Aliasbidder fix (prebid#1652) prebidAdapter secure support (prebid#1655) Increment pre version Prebid 0.30.0 Release Add native param support to mediaTypes (prebid#1625) PulsePoint Lite adpater changes (prebid#1630) Appnexus ast unittest updates (prebid#1654) Support aspect ratio specification for native images (prebid#1634) Revert changes for switch between client side and server side. (prebid#1653) rubicon converted to bidderFactory (prebid#1624) Add JSDoc for `pbjs.getAllWinningBids` (prebid#1566) Add ignore-loader to handle .md files (prebid#1646) fixed PBS cookie syncs (prebid#1637) Add placementId request param to Yieldmo bid adapter (prebid#1632) Adxcg analytics adapter (prebid#1599) Add publisher sub-id support to the Criteo adapter (prebid#1629) ...
* aliasBidder did not work for bidderFactory * added function to get adapter spec * Freezing spec and moved unit tests * Updated test case to not import adapter
Type of change
Description of change
Fix for #1643
Also, aliased adapter could only support banner mediaType. Fixed that too.