Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

s2sTesting: random number moved to global #3851

Merged
merged 6 commits into from
Aug 21, 2019
Merged

s2sTesting: random number moved to global #3851

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5e3ad83
s2sTesting: random number moved to global
bretg May 22, 2019
3a47c7c
Added get for randNum + fix lint & tests
robertrmartinez May 23, 2019
ab7c091
Merge branch 'master' of https://github.com/prebid/Prebid.js into s2s…
robertrmartinez Aug 12, 2019
24cadaa
update to remove getter
robertrmartinez Aug 12, 2019
dcbb0c0
remove log
robertrmartinez Aug 12, 2019
2ed1e24
update test
robertrmartinez Aug 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion modules/s2sTesting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ s2sTesting.CLIENT = CLIENT;

var testing = false; // whether testing is turned on
var bidSource = {}; // store bidder sources determined from s2sConfing bidderControl
var globalRand = Math.random(); // if 10% of bidderA and 10% of bidderB should be server-side, make it the same 10%

// load s2sConfig
config.getConfig('s2sConfig', config => {
Expand Down Expand Up @@ -82,14 +83,18 @@ s2sTesting.getSource = function(sourceWeights = {}, bidSources = [SERVER, CLIENT
});
if (!totWeight) return; // bail if no source weights
// choose a source randomly based on weights
var rndWeight = Math.random() * totWeight;
var rndWeight = s2sTesting.getGlobalRand() * totWeight;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why using a getter when you can access globaRand directly?

for (var i = 0; i < bidSources.length; i++) {
let source = bidSources[i];
// choose the first source with an incremental weight > random weight
if (rndWeight < srcIncWeight[source]) return source;
}
};

s2sTesting.getGlobalRand = function() {
return globalRand;
};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need this getter when globaRand is never publicly accessed? Am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it just for testing?

// inject the s2sTesting module into the adapterManager rather than importing it
// importing it causes the packager to include it even when it's not explicitly included in the build
setS2STestingModule(s2sTesting);
Expand Down
33 changes: 27 additions & 6 deletions test/spec/modules/s2sTesting_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
import s2sTesting from 'modules/s2sTesting';
import { config } from 'src/config';
import find from 'core-js/library/fn/array/find';

var events = require('src/events');
var CONSTANTS = require('src/constants.json');
const BID_ADJUSTMENT = CONSTANTS.EVENTS.BID_ADJUSTMENT;

var expect = require('chai').expect;

Expand All @@ -13,7 +8,7 @@ describe('s2sTesting', function () {
let randomNumber = 0;

beforeEach(function () {
mathRandomStub = sinon.stub(Math, 'random').callsFake(() => { return randomNumber; });
mathRandomStub = sinon.stub(s2sTesting, 'getGlobalRand').callsFake(() => { return randomNumber; });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as per the comments above, not sure this is needed. thinking you can just stick with stubbing Math.random

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or is this for testing?

});

afterEach(function () {
Expand Down Expand Up @@ -155,6 +150,32 @@ describe('s2sTesting', function () {
expect(serverClientBidders.server).to.eql(['rubicon']);
expect(serverClientBidders.client).to.have.members(['appnexus']);
});

it('sends both bidders to same source when weights are the same', function () {
config.setConfig({s2sConfig: {
bidders: ['rubicon', 'appnexus'],
testing: true,
bidderControl: {
rubicon: {bidSource: {server: 25, client: 75}},
appnexus: {bidSource: {server: 25, client: 75}}
}}});
expect(s2sTesting.getSourceBidderMap()).to.eql({
client: ['rubicon', 'appnexus'],
server: []
});

config.setConfig({s2sConfig: {
bidders: ['rubicon', 'appnexus'],
testing: true,
bidderControl: {
rubicon: {bidSource: {server: 75, client: 25}},
appnexus: {bidSource: {server: 75, client: 25}}
}}});
expect(s2sTesting.getSourceBidderMap()).to.eql({
server: ['rubicon', 'appnexus'],
client: []
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these really test your change since you've fixed the random number for the tests. I believe they would've always passed before you made the code change.

I think a better test would be to call s2sTesting.getSourceBidderMap() multiple times, changing the random number each time, and verifying that the result matches what you'd expect from the first random number since it shouldn't change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but because you're using the using and stubbing the global getter, changing the random number is irrelevant. so not sure how you test.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe for these tests you remove the getter stub, add the Math.random stub, and verify that only the original random number matters.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harpere

So the problem is that if we leave var globalRand = Math.random();

Then the test cannot change the value, it is being set just once, and all tests use the same value.

Our sinon stub for Math Random does not trigger before the module initializes the random number.

Not sure the best approach to this testing wise.

});

describe('setting source through adUnits', function () {
Expand Down