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

Fix Currency Converter Doesn't Output CUR #1154

Merged
merged 9 commits into from
Jan 10, 2020
Merged

Fix Currency Converter Doesn't Output CUR #1154

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d3f5c06
currencies/rate_converter_test.go test case
Dec 20, 2019
a099405
Solution and test case update
Dec 21, 2019
69425e6
exchange/exchange_test.go ready
Dec 21, 2019
5915a78
Added test on the adform adapter itself
Dec 21, 2019
e2116e2
added comments to exchange_test.go
Dec 21, 2019
a82cc58
Corrections to address PR comments
Jan 7, 2020
158bc22
Correct conflict
Jan 9, 2020
c57b31a
Correct some more conflicts
Jan 9, 2020
e8e6cee
Merge branch 'master' into PBS-574
guscarreon Jan 9, 2020
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
80 changes: 80 additions & 0 deletions adapters/adform/adform_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import (
"github.com/prebid/prebid-server/adapters"
"github.com/prebid/prebid-server/config"
"github.com/prebid/prebid-server/openrtb_ext"

"github.com/stretchr/testify/assert"
)

func TestJsonSamples(t *testing.T) {
Expand Down Expand Up @@ -597,3 +599,81 @@ func TestPriceTypeUrlParameterCreation(t *testing.T) {
}
}
}

// Asserts that toOpenRtbBidResponse() creates a *adapters.BidderResponse with
// the currency of the last valid []*adformBid element and the expected number of bids
func TestToOpenRtbBidResponse(t *testing.T) {
// expected values
Copy link
Contributor

Choose a reason for hiding this comment

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

The flow of this test is very clean. I recommend to remove all of the comments, I don't think they're necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

expectedBids := 3
lastCurrency, anotherCurrency, emptyCurrency := "EUR", "USD", ""

// init `toOpenRtbBidResponse` parameters
request := &openrtb.BidRequest{
ID: "test-request-id",
Imp: []openrtb.Imp{
{ //1
ID: "banner-imp-no1",
Ext: json.RawMessage(`{"bidder1": { "mid": "32341" }}`),
Banner: &openrtb.Banner{},
},
{ //2
ID: "banner-imp-no2",
Ext: json.RawMessage(`{"bidder1": { "mid": "32342" }}`),
Banner: &openrtb.Banner{},
},
{ //3
ID: "banner-imp-no3",
Ext: json.RawMessage(`{"bidder1": { "mid": "32343" }}`),
Banner: &openrtb.Banner{},
},
{ //4
ID: "banner-imp-no4",
Ext: json.RawMessage(`{"bidder1": { "mid": "32344" }}`),
Banner: &openrtb.Banner{},
},
},
Device: &openrtb.Device{UA: "ua", IP: "ip"},
User: &openrtb.User{BuyerUID: "buyerUID"},
}

testAdformBids := []*adformBid{
{ //1
ResponseType: "banner",
Banner: "banner-content1",
Price: 1.23,
Currency: anotherCurrency,
Width: 300,
Height: 200,
DealId: "dealId1",
CreativeId: "creativeId1",
},
{}, //2
{ //3
ResponseType: "banner",
Banner: "banner-content2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider naming this banner-content3, dealId3, and creativeId3 to better align with the impression numbers above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Price: 1.24,
Currency: emptyCurrency,
Width: 300,
Height: 200,
DealId: "dealId2",
CreativeId: "creativeId2",
},
{ //4
ResponseType: "banner",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same deal, with banner-content4, etc...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Banner: "banner-content3",
Price: 1.25,
Currency: lastCurrency,
Width: 300,
Height: 200,
DealId: "dealId3",
CreativeId: "creativeId3",
},
}

// Execute `toOpenRtbBidResponse`
actualBidResponse := toOpenRtbBidResponse(testAdformBids, request)

// Assert number of bids and currency
assert.Equalf(t, expectedBids, len(actualBidResponse.Bids), fmt.Sprintf("[TestToOpenRtbBidResponse] actualBidResponse lenght differs\n"))
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to use fmt.Sprintf for a literal string. You can also omit the test name since the framework provides enough context. The framework also provides information about why the equality test failed, so consider using the description to just capture what you're testing like "Bid Count" and "Currency".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

assert.Equalf(t, lastCurrency, actualBidResponse.Currency, fmt.Sprintf("[TestToOpenRtbBidResponse] actualBidResponse lenght differs\n"))
}
1 change: 1 addition & 0 deletions currencies/rate_converter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,7 @@ func TestRates(t *testing.T) {
{from: "", to: "EUR", expectedRate: 0, hasError: true},
{from: "CNY", to: "", expectedRate: 0, hasError: true},
{from: "", to: "", expectedRate: 0, hasError: true},
{from: "USD", to: "USD", expectedRate: 1, hasError: false},
}

mockedHttpServer := httptest.NewServer(http.HandlerFunc(
Expand Down
1 change: 1 addition & 0 deletions exchange/exchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,7 @@ func (e *exchange) buildBidResponse(ctx context.Context, liveAdapters []openrtb_
if adapterBids[a] != nil && len(adapterBids[a].bids) > 0 {
sb := e.makeSeatBid(adapterBids[a], a, adapterExtra, auc)
seatBids = append(seatBids, *sb)
bidResponse.Cur = adapterBids[a].currency
}
}

Expand Down
160 changes: 127 additions & 33 deletions exchange/exchange_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,51 +314,145 @@ func TestGetBidCacheInfo(t *testing.T) {
assert.Equal(t, expCacheURL, cacheURL, "[TestGetBidCacheInfo] cacheId field in ext should equal \"%s\" \n", expCacheURL)
}

func buildBidResponseParams(bidderName openrtb_ext.BidderName, bidRequest *openrtb.BidRequest) ([]openrtb_ext.BidderName, map[openrtb_ext.BidderName]*pbsOrtbSeatBid, json.RawMessage, map[openrtb_ext.BidderName]*seatResponseExtra) {
//liveAdapters []openrtb_ext.BidderName,
liveAdapters := []openrtb_ext.BidderName{bidderName}
func TestBidResponseCurrency(t *testing.T) {
// Init objects
cfg := &config.Configuration{Adapters: make(map[string]config.Adapter, 1)}
cfg.Adapters["appnexus"] = config.Adapter{Endpoint: "http://ib.adnxs.com"}

//adapterBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid,
adapterBids := map[openrtb_ext.BidderName]*pbsOrtbSeatBid{
bidderName: {
bids: []*pbsOrtbBid{
{
bid: &openrtb.Bid{
ID: "some-imp-id",
Price: 9.517803,
W: 300,
H: 250,
},
bidType: openrtb_ext.BidTypeBanner,
bidTargets: map[string]string{
"pricegranularity": "med",
"includewinners": "true",
"includebidderkeys": "false",
},
},
},
currency: "USD",
//ext: bidRequest.Ext,
},
handlerNoBidServer := func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(204) }
server := httptest.NewServer(http.HandlerFunc(handlerNoBidServer))
defer server.Close()

e := NewExchange(server.Client(), nil, cfg, pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList(), config.DisabledMetrics{}), adapters.ParseBidderInfos(cfg.Adapters, "../static/bidder-info", openrtb_ext.BidderList()), gdpr.AlwaysAllow{}, currencies.NewRateConverterDefault()).(*exchange)

liveAdapters := make([]openrtb_ext.BidderName, 1)
liveAdapters[0] = "appnexus"

bidRequest := &openrtb.BidRequest{
ID: "some-request-id",
Imp: []openrtb.Imp{{
ID: "some-impression-id",
Banner: &openrtb.Banner{Format: []openrtb.Format{{W: 300, H: 250}, {W: 300, H: 600}}},
Ext: json.RawMessage(`{"appnexus": {"placementId": 10433394}}`),
}},
Site: &openrtb.Site{Page: "prebid.org", Ext: json.RawMessage(`{"amp":0}`)},
Device: &openrtb.Device{UA: "curl/7.54.0", IP: "::1"},
AT: 1,
TMax: 500,
Ext: json.RawMessage(`{"id": "some-request-id","site": {"page": "prebid.org"},"imp": [{"id": "some-impression-id","banner": {"format": [{"w": 300,"h": 250},{"w": 300,"h": 600}]},"ext": {"appnexus": {"placementId": 10433394}}}],"tmax": 500}`),
}

//resolvedRequest json.RawMessage
resolvedRequest := json.RawMessage(`{"id": "some-request-id","site": {"page": "prebid.org"},"imp": [{"id": "some-impression-id","banner": {"format": [{"w": 300,"h": 250},{"w": 300,"h": 600}]},"ext": {"appnexus": {"placementId": 10433394}}}],"tmax": 500}`)

//adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra,
adapterExtra := map[openrtb_ext.BidderName]*seatResponseExtra{
bidderName: {
ResponseTimeMillis: 5,
Errors: []openrtb_ext.ExtBidderError{
"appnexus": {ResponseTimeMillis: 5},
}

var errList []error

sampleBid := &openrtb.Bid{
ID: "some-imp-id",
Price: 9.517803,
W: 300,
H: 250,
Ext: nil,
}
aPbsOrtbBidArr := []*pbsOrtbBid{{bid: sampleBid, bidType: openrtb_ext.BidTypeBanner}}
sampleSeatBid := []openrtb.SeatBid{
{
Seat: string("appnexus"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it need to be explicitly set as a string type like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it's not needed. Corrected

Bid: []openrtb.Bid{
{
Code: 999,
Message: "Post ib.adnxs.com/openrtb2?query1&query2: unsupported protocol scheme \"\"",
ID: "some-imp-id",
Price: 9.517803,
W: 300,
H: 250,
Ext: json.RawMessage(`{"prebid":{"type":"banner"}}`),
},
},
},
}
emptySeatBid := []openrtb.SeatBid{}

return liveAdapters, adapterBids, resolvedRequest, adapterExtra
// Test cases
type aTest struct {
description string
adapterBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid
expectedBidResponse *openrtb.BidResponse
}
testCases := []aTest{
{
description: "1) Adapter to bids map comes with a non-empty currency field and non-empty bid array",
adapterBids: map[openrtb_ext.BidderName]*pbsOrtbSeatBid{
openrtb_ext.BidderName("appnexus"): {
bids: aPbsOrtbBidArr,
currency: "USD",
},
},
expectedBidResponse: &openrtb.BidResponse{
ID: "some-request-id",
SeatBid: sampleSeatBid,
Cur: "USD",
Ext: json.RawMessage(`{"responsetimemillis":{"appnexus":5},"tmaxrequest":500}
`),
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider moving these terminal `')' pairs to the end of the line. Might look cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If instead of:

    Ext: json.RawMessage(`{"responsetimemillis":{"appnexus":5},"tmaxrequest":500}
`),

I put everything in a single line:

    Ext:     json.RawMessage(`{"responsetimemillis":{"appnexus":5},"tmaxrequest":500}`),

Tht test faisl because we are missing a line break:

Messages:       [TEST_FAILED] Objects must be equal for test: 1) Adapter to bids map comes with a non-empty currency field and non-empty bid array
                 Expected: >>{"responsetimemillis":{"appnexus":5},"tmaxrequest":500}<<
                 Actual: >>{"responsetimemillis":{"appnexus":5},"tmaxrequest":500}
                <<

I googled this issue and apparently JSON should recognize the common line break escape sequence \n and even \u000A, but we still fail the test:

Messages:       [TEST_FAILED] Objects must be equal for test: 1) Adapter to bids map comes with a non-empty currency field and non-empty bid array
                 Expected: >>{"responsetimemillis":{"appnexus":5},"tmaxrequest":500}\n<<
                 Actual: >>{"responsetimemillis":{"appnexus":5},"tmaxrequest":500}
                <<

Have you come accross this before?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok. The response must include a line break at the end. Got it. That's fine. Please just resolve the merge conflict and we'll be good here.

},
},
{
description: "2) Adapter to bids map comes with a non-empty currency field but an empty bid array",
adapterBids: map[openrtb_ext.BidderName]*pbsOrtbSeatBid{
openrtb_ext.BidderName("appnexus"): {
bids: nil,
currency: "USD",
},
},
expectedBidResponse: &openrtb.BidResponse{
ID: "some-request-id",
SeatBid: emptySeatBid,
Cur: "",
Ext: json.RawMessage(`{"responsetimemillis":{"appnexus":5},"tmaxrequest":500}
`),
},
},
{
description: "3) Adapter to bids map comes with an empty currency string and a non-empty bid array",
adapterBids: map[openrtb_ext.BidderName]*pbsOrtbSeatBid{
openrtb_ext.BidderName("appnexus"): {
bids: aPbsOrtbBidArr,
currency: "",
},
},
expectedBidResponse: &openrtb.BidResponse{
ID: "some-request-id",
SeatBid: sampleSeatBid,
Cur: "",
Ext: json.RawMessage(`{"responsetimemillis":{"appnexus":5},"tmaxrequest":500}
`),
},
},
{
description: "4) Adapter to bids map comes with an empty currency string and an empty bid array",
adapterBids: map[openrtb_ext.BidderName]*pbsOrtbSeatBid{
openrtb_ext.BidderName("appnexus"): {
bids: nil,
currency: "",
},
},
expectedBidResponse: &openrtb.BidResponse{
ID: "some-request-id",
SeatBid: emptySeatBid,
Cur: "",
Ext: json.RawMessage(`{"responsetimemillis":{"appnexus":5},"tmaxrequest":500}
`),
},
},
}

// Run tests
for i := range testCases {
actualBidResp, err := e.buildBidResponse(context.Background(), liveAdapters, testCases[i].adapterBids, bidRequest, resolvedRequest, adapterExtra, nil, errList)
assert.NoError(t, err, fmt.Sprintf("[TEST_FAILED] e.buildBidResponse resturns error in test: %s Error message: %s \n", testCases[i].description, err))
assert.Equalf(t, testCases[i].expectedBidResponse, actualBidResp, fmt.Sprintf("[TEST_FAILED] Objects must be equal for test: %s \n Expected: >>%s<< \n Actual: >>%s<< ", testCases[i].description, testCases[i].expectedBidResponse.Ext, actualBidResp.Ext))
}
}

// TestRaceIntegration runs an integration test using all the sample params from
Expand Down