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.

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

Rubicon Adapter - SRA support legacy #2479

Merged

Conversation

idettman
Copy link
Contributor

@idettman idettman commented May 2, 2018

Type of change

  • Feature

Description of change

Adds "Single Request Architecture" (SRA) support to the Rubicon Adapter.

// By default, SRA mode is turned off

// Enable SRA with the following config change
pbjs.setConfig({
  rubicon: {
    singleRequest: true
  }
});
  • test parameters for validating bids
const exampleBids = [
{
  code: 'div-1',
  sizes: [[728, 90], [970, 90]],
  bids: [
    {
      bidder: 'rubicon',
      params: {
        accountId: "1234",
        siteId: "1234",
        zoneId: "1001"
    },
    {
      bidder: 'rubicon',
      params: {
        accountId: "1234",
        siteId: "1234",
        zoneId: "1002",
    },
    {
      bidder: 'rubicon',
      params: {
        accountId: "1234",
        siteId: "9876", // Note: <-- this siteId doesn't match the other two bids, so a second request is sent for bids with that 'siteId' value
        zoneId: "1003"
    }]
}];

Other information

HB-2542

@mike-chowla
Copy link
Contributor

@idettman There's a merge conflict on this PR. Can you resolve it?

@mike-chowla
Copy link
Contributor

There's no SRA tests included which is a bit surprising because the equvalent PR (#2478) for master includes

@mike-chowla mike-chowla self-requested a review May 12, 2018 00:04
Copy link
Contributor

@mike-chowla mike-chowla left a comment

Choose a reason for hiding this comment

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

Same comments as #2478

The merge conflict needs to be fixed.
The test parameters object is in the description is missing 3 '}' (one for each params object)
The test parameters return errors (which are the same errors as I get on #2478)

@idettman
Copy link
Contributor Author

idettman commented May 17, 2018

We still need to enable sra on our endpoint, I'll reply with an update

@idettman idettman requested a review from snapwich May 17, 2018 19:43
Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

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

LGTM

@mike-chowla
Copy link
Contributor

Since Travis failed (due to out of memory error), I ran the tests locally and I'm getting test errors on the adapter:

[15:56:38] Starting 'test-coverage'...
23 05 2018 15:57:52.766:INFO [karma]: Karma v1.7.1 server started at http://0.0.0.0:9876/
23 05 2018 15:57:52.769:INFO [launcher]: Launching browser ChromeHeadless with unlimited concurrency
23 05 2018 15:57:52.835:INFO [launcher]: Starting browser ChromeHeadless
23 05 2018 15:57:53.720:INFO [HeadlessChrome 66.0.3359 (Mac OS X 10.12.6)]: Connected on socket y9N801Q4zMN_WSgvAAAA with id 76817521
HeadlessChrome 66.0.3359 (Mac OS X 10.12.6): Executed 48 of 1835 SUCCESS (0 secs / 0.03 secs)
HeadlessChrome 66.0.3359 (Mac OS X 10.12.6) the rubicon adapter buildRequests implementation for requests for video requests should make a well-formed video request with legacy mediaType config FAILED
	TypeError: Cannot read property 'replace' of undefined
	    at Object.exports.stringify (webpack:///node_modules/mocha/mocha.js:5851:0 <- test/spec/videoCache_spec.js:9793:60)
	    at processAssertionError (node_modules/karma-mocha/lib/adapter.js:59:41)
	    at Runner.<anonymous> (node_modules/karma-mocha/lib/adapter.js:129:28)
HeadlessChrome 66.0.3359 (Mac OS X 10.12.6): Executed 1173 of 1835 (1 FAILED) (0 secs / 1.126 secs)
HeadlessChrome 66.0.3359 (Mac OS X 10.12.6) the rubicon adapter buildRequests implementation for requests for video requests should make a well-formed video request with legacy mediaType config FAILED
	TypeError: Cannot read property 'replace' of undefined
	    at Object.exports.stringify (webpack:///node_modules/mocha/mocha.js:5851:0 <- test/spec/videoCache_spec.js:9793:60)
	    at processAssertionError (node_modules/karma-mocha/lib/adapter.js:59:41)
HeadlessChrome 66.0.3359 (Mac OS X 10.12.6) the rubicon adapter buildRequests implementation for requests for video requests should make a well-formed video request FAILED
	TypeError: Cannot read property 'replace' of undefined
	    at Object.exports.stringify (webpack:///node_modules/mocha/mocha.js:5851:0 <- test/spec/videoCache_spec.js:9793:60)
	    at processAssertionError (node_modules/karma-mocha/lib/adapter.js:59:41)
	    at Runner.<anonymous> (node_modules/karma-mocha/lib/adapter.js:129:28)
HeadlessChrome 66.0.3359 (Mac OS X 10.12.6): Executed 1174 of 1835 (2 FAILED) (0 secs / 1.128 secs)
HeadlessChrome 66.0.3359 (Mac OS X 10.12.6) the rubicon adapter buildRequests implementation for requests for video requests should make a well-formed video request FAILED
	TypeError: Cannot read property 'replace' of undefined
	    at Object.exports.stringify (webpack:///node_modules/mocha/mocha.js:5851:0 <- test/spec/videoCache_spec.js:9793:60)
	    at processAssertionError (node_modules/karma-mocha/lib/adapter.js:59:41)
HeadlessChrome 66.0.3359 (Mac OS X 10.12.6): Executed 1293 of 1835 (2 FAILED) (0 secs / 1.188 secs)
HeadlessChrome 66.0.3359 (Mac OS X 10.12.6): Executed 1835 of 1835 (2 FAILED) (12.058 secs / 2.003 secs)

=============================== Coverage summary ===============================
Statements   : 89.23% ( 10328/11575 )
Branches     : 76.77% ( 4964/6466 )
Functions    : 91.11% ( 1762/1934 )
Lines        : 89.38% ( 10104/11305 )
================================================================================
23 05 2018 15:58:24.695:WARN [launcher]: ChromeHeadless was not killed in 2000 ms, sending SIGKILL.
[15:58:24] 'test-coverage' errored after 1.77 min
[15:58:24] Error: Karma tests failed with exit code 1
    at /Users/mike.chowla/src/prebid-legacy/Prebid.js/gulpfile.js:120:12
    at removeAllListeners (/Users/mike.chowla/src/prebid-legacy/Prebid.js/node_modules/karma/lib/server.js:380:7)
    at Server.<anonymous> (/Users/mike.chowla/src/prebid-legacy/Prebid.js/node_modules/karma/lib/server.js:391:9)
    at Object.onceWrapper (events.js:313:30)
    at emitNone (events.js:111:20)
    at Server.emit (events.js:208:7)
    at emitCloseNT (net.js:1655:8)
    at _combinedTickCallback (internal/process/next_tick.js:135:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)

@mike-chowla
Copy link
Contributor

I'm heading out for a vacation early tomorrow morning. Please feel free to ask for this to reassigned once the tests are fixed

Copy link
Collaborator

@harpere harpere left a comment

Choose a reason for hiding this comment

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

LGTM

@mike-chowla mike-chowla merged commit e9de33c into prebid:legacy Jun 11, 2018
@idettman idettman deleted the rubicon-adapter-sra-rebase-legacy branch February 5, 2019 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants