added new rubiconLite adapter

[snapwich]

Type of change

• New bidder adapter

Description of change

This is a lightweight Rubicon adapter that no longer includes the Fastlane library when participating in an auction. Due to the lack of Fastlane library, some APIs are unavailable (such as those on the rubicontag). For those who still require access to those APIs they can use the regular rubicon adapter; for those that don't need those APIs, there is now the option to use this rubiconLite adapter for a performance increase.

• test parameters for validating bids

{
  bidder: 'rubiconLite',
  params: {
    // ... (same as regular Rubicon adapter)
  }
}

[mkendall07]

Looks like you have a unit test failure. Once it's addressed I'll review.

[snapwich]

@mkendall07 should be good now. saucelabs didn't like my mocked window.screen so I just replaced that with a regexp check and it seems to be happy now.

[mkendall07]

@snapwich
Thanks for the PR. I've put some initial comments for you to review/address. I'll validate bids after updates. Thanks.

[mkendall07]

We prefer now to use the ES6 import syntax. See https://github.com/prebid/Prebid.js/blob/master/src/adapters/appnexusAst.js for examples

[mkendall07]

I'm not clear why mockResponse is passed to the constructor function?

[snapwich]

It was my method of testing only the ajax callback in unit tests. It could be written in a way to expose the callback as an export. Do you have a preference?

[mkendall07]

use CONSTANTS.STATUS.NO_BID instead of 2.

[mkendall07]

Prefer ES6 template literals here.

[mkendall07]

this doesn't actually do anything. Please use CONSTANTS.STATUS.GOOD on line 174 instead.

[mkendall07]

nit: this comment isn't really accurate.

[snapwich]

submitted updates

[mkendall07]

Thanks @snapwich
Sorry to push the issue but what's the deal with mockResponse ? I'm not a fan of writing code for prod that is only for test. Maybe I'm missing something though.

[snapwich]

I can't think of any other methods to test the ajax callback in isolation without either passing in a testing dependency or exposing some private methods as exports, both of which require extra code within the module that is really only for testing. Do you have a preference?

[mkendall07]

right that's because ES6 modules are read only which is a pain. To get around these, we used sinon.fakeServer to mock the response that the ajax function returns. Would this work? Example: https://github.com/prebid/Prebid.js/blob/master/test/spec/adapters/appnexusAst_spec.js#L163

[snapwich]

Unfortunately mocking the AJAX call is insufficient to test the callback in isolation since the only access is through callBids. Isolation is pretty important in this case I think due to the fact that we have a lot more response handling tests than request sending tests. Without isolation it requires the response handling tests to run the request handling code with each test which is both slow and prone to causing all response tests to fail from request bugs (which makes debugging difficult).

I recognize the way I have it now isn't even completely isolated since the test call is still made through callBids (although quickly directed to the callback). I think the ideal situation, and what I've done in the past, is to pass a testing flag using the Webpack DefinePlugin. This allows testing code to be present in the module if(TESTING) { which translates to if(true/false) { which uglify will then remove as dead code in production builds. I think in this case it would be easy to implement since you have a different execution path for testing vs production builds, would you like me to take a crack at it?

[mkendall07]

@snapwich
I disagree with

mocking the AJAX call is insufficient to test the callback in isolation since the only access is through callBids.

We tend to subscribe to "test behaviors, not implementations" concept. If your in a situation where you find yourself having to test private methods, you are unit testing implementation vs public interface. Here's a link: https://teamgaslight.com/blog/testing-behavior-vs-testing-implementation

I'm not sure if writing conditionals into the code for test/prod is more desirable than the current way you are doing it.

What this means practically, is that you should be able to test what is needed by the public interface by mocking out dependencies as needed.

[snapwich]

We tend to subscribe to "test behaviors, not implementations" concept. If your in a situation where you find yourself having to test private methods, you are unit testing implementation vs public interface. Here's a link: https://teamgaslight.com/blog/testing-behavior-vs-testing-implementation

I agree with your statement and the article you've provided. Unfortunately AJAX has the side-effect of exposing an additional API which is public in the sense that an external resource has to communicate with our internal implementation. By "exposing" that hidden API in the way prescribed (fake server responses) it is requiring the unit tests to execute a lot of additional code that is not under test which has negative consequences for both testing performance and false-positives when not-under-test-code has bugs.

However, in interest of moving forward, I've made the recommendation here as suggested. I hope you'll consider allowing testing and debugging code paths in the future for builds as I have seen many positive benefits in the past.

[mkendall07]

thanks for the update.
Regarding

allowing testing and debugging code paths in the future for builds

I'm hesitant to allow it because I think it can encourage bad behavior "it's OK because it's test code and thrown away anyway" type of attitude. Not that it cannot be done right. We'll always consider other's viewpoints on these things. Thanks for contributing.