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 all 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
3 changes: 2 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
s2sTesting.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,7 +83,7 @@ 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.globalRand * totWeight;
for (var i = 0; i < bidSources.length; i++) {
let source = bidSources[i];
// choose the first source with an incremental weight > random weight
Expand Down
68 changes: 48 additions & 20 deletions test/spec/modules/s2sTesting_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,14 @@
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;

describe('s2sTesting', function () {
let mathRandomStub;
let randomNumber = 0;

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

afterEach(function () {
mathRandomStub.restore();
});

describe('s2sTesting.getSource', function () {
// helper function to set random number and get the source
function getExpectedSource(randNumber, sourceWeights, sources) {
// set random number for testing
randomNumber = randNumber;
s2sTesting.globalRand = randNumber;
return s2sTesting.getSource(sourceWeights, sources);
}

Expand Down Expand Up @@ -93,7 +77,7 @@ describe('s2sTesting', function () {
describe('setting source through s2sConfig', function () {
beforeEach(function () {
// set random number for testing
randomNumber = 0.7;
s2sTesting.globalRand = 0.7;
});

it('does not work if testing is "false"', function () {
Expand Down Expand Up @@ -155,14 +139,58 @@ 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 () {
s2sTesting.globalRand = 0.01;

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

config.setConfig({s2sConfig: {
bidders: ['rubicon', 'appnexus'],
testing: true,
bidderControl: {
rubicon: {bidSource: {server: 99, client: 1}},
appnexus: {bidSource: {server: 99, client: 1}}
}}});
expect(s2sTesting.getSourceBidderMap()).to.eql({
server: ['rubicon', 'appnexus'],
client: []
});
expect(s2sTesting.getSourceBidderMap()).to.eql({
server: ['rubicon', 'appnexus'],
client: []
});
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 () {
beforeEach(function () {
// reset s2sconfig bid sources
config.setConfig({s2sConfig: {testing: true}});
// set random number for testing
randomNumber = 0.7;
s2sTesting.globalRand = 0.7;
});

it('sets one bidder source from one adUnit', function () {
Expand Down Expand Up @@ -280,7 +308,7 @@ describe('s2sTesting', function () {
// reset s2sconfig bid sources
config.setConfig({s2sConfig: {testing: true}});
// set random number for testing
randomNumber = 0.7;
s2sTesting.globalRand = 0.7;
});

it('should get sources from both', function () {
Expand Down