-
Notifications
You must be signed in to change notification settings - Fork 732
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
Log non bid reasons in bidder framework #2891
Log non bid reasons in bidder framework #2891
Conversation
exchange/non_bid_reason.go
Outdated
@@ -22,3 +25,11 @@ func (n *NonBidReason) Val() NonBidReason { | |||
} | |||
return *n | |||
} | |||
|
|||
func ErrorToNonBidReason(errorCode int) NonBidReason { | |||
switch errorCode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you anticipate more cases to be added to this switch statement in the future? If not, can this be simplified to just check if errorCode == errortypes.TimeoutErrorCode
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I could see we will need to incorporate more cases here in future based on the Seat Non Bid community extension
exchange/non_bid_reason.go
Outdated
func ErrorToNonBidReason(errorCode int) NonBidReason { | ||
switch errorCode { | ||
case errortypes.TimeoutErrorCode: | ||
return ErrorTimeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And is there a way to test this function so that this line is covered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Covered via
prebid-server/exchange/bidder_test.go
Line 3060 in d4754a1
func TestSeatNonBid(t *testing.T) { |
@ShriprasadM Just pinging this PR to see if you'll be able to address my comments |
@AlexBVolcy : As per discussion on bi-weekly meeting, when can we expect findings around above proposal? |
Yes. please check the updated PR |
exchange/bidder_test.go
Outdated
// seatBids: []*entities.PbsOrtbSeatBid{{Bids: []*entities.PbsOrtbBid{}, Currency: "USD", Seat: "someseat", HttpCalls: []*openrtb_ext.ExtHttpCall{}}}, | ||
}, | ||
}, /* { | ||
name: "103_bidder_unreachable", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this test commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to make core unit test implementation reusable for future error code. will revert it for now
exchange/exchange.go
Outdated
newErr := openrtb_ext.ExtBidderMessage{ | ||
Code: errortypes.ReadCode(err), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this change made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted this change. It is unwanted. Initially I was trying to accommodate, seatnonbid related changes here
exchange/exchange.go
Outdated
@@ -1582,5 +1595,8 @@ func setSeatNonBid(bidResponseExt *openrtb_ext.ExtBidResponse, seatNonBids nonBi | |||
} | |||
|
|||
bidResponseExt.Prebid.SeatNonBid = seatNonBids.get() | |||
if adapterNonBids != nil { | |||
bidResponseExt.Prebid.SeatNonBid = append(bidResponseExt.Prebid.SeatNonBid, adapterNonBids...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this functionality tested anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested locally. But will add the unit test for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Left one comment
@@ -3056,3 +3056,91 @@ func TestGetBidType(t *testing.T) { | |||
}) | |||
} | |||
} | |||
|
|||
func TestSeatNonBid(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any other test cases you can think of that would make sense here? I only ask since I see only one test case and was wondering if there are any others that would make sense.
@ShriprasadM just pinging to see if you've seen the above comments made? And if you could merge with master to fix conflicts that'd be great! |
This PR will require that adapters return imp IDs from MakeRequests so that we know what errors go with which Imps. Blocked until a separate PR adds imps to MakeRequests return values. |
@hhhjort and @SyntaxNode : I have created separate issue - #2973 for this |
exchange/bidder.go
Outdated
Seat: string(bidderRequest.BidderName), | ||
NonBid: make([]openrtb_ext.NonBid, len(bidderRequest.BidRequest.Imp)), | ||
} | ||
for i, imp := range bidderRequest.BidRequest.Imp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SyntaxNode and team:
If you look at the above code snippet, we are applying nonBidReason
flat across all impressions here. As, we are not able to identify per impression level scenario here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SyntaxNode / @hhhjort : Can you update here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this address by #2973?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SyntaxNode : #2973 will address adapter level changes. Until, adapters are not providing impression level information in RequestData
, this block of implementation will apply nonBidReason
flat across all impressions.
So, I am proposing, till #2973 not effective, can we move on with the above assumption. I know there will be ambiguous cases, where adapter may have bided for the impression but still will add it under non bid because other impression may have timed out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can discuss at our next catchup. I was under the impression we decided to pause this issue until we implement #2973. Then we can avoid ambiguous cases.
Prebid Server 2.0 has been released and Go Module name has changed from Please merge the |
Blocked By #3364 |
@SyntaxNode can you check my replies on review comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShriprasadM Scott and I discussed offline. Please see my latest comments.
|
||
const ( | ||
NoBidUnknownError NonBidReason = 0 // No Bid - General |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Please bring this back and update all of the non bid reasons on lines 17-25 to be of type NonBidReason
. It is my understanding that the openrtb3.NoBidReason
type is used to enumerate no bid reason codes sent back from a bidder in its response. Those reason codes will only ever be between 0-99. It is true that those no bid reason codes may end up being reported by Prebid Server as non bid reasons but they should be mapped from the no bid reason enumeration to the non bid reason enumeration even though the numbers line up since the enumerations have different meaning and are not identical. The non bid reasons could also end up being any of these Prebid Server specific codes starting at 100.
exchange/non_bid_reason.go
Outdated
} | ||
|
||
// isBidderUnreachableError checks if the error is due to connection refused or no such host | ||
func isBidderUnreachableError(httpInfo *httpCallInfo) bool { | ||
return errors.Is(httpInfo.err, syscall.ECONNREFUSED) || strings.Contains(httpInfo.err.Error(), "no such host") | ||
var dnsErr *net.DNSError | ||
return errors.Is(httpInfo.err, syscall.ECONNREFUSED) || (errors.As(httpInfo.err, &dnsErr) && dnsErr.IsNotFound) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit cryptic. I prefer using the temporary self explanatory variable @SyntaxNode suggested:
isNoSuchHost := errors.As(err, &dnsErr) && dnsErr.IsNotFound
return errors.Is(httpInfo.err, syscall.ECONNREFUSED) || isNoSuchHost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
exchange/seat_non_bids.go
Outdated
// get returns a slice of SeatNonBid objects representing the non-bids for each seat. | ||
// If snb is nil, it returns nil. | ||
// It iterates over the seatNonBidsMap and appends each seat and its corresponding non-bids to the seatNonBid slice. | ||
// Finally, it returns the seatNonBid slice. | ||
func (snb *nonBids) get() []openrtb_ext.SeatNonBid { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SyntaxNode and I agree that this should be refactored. This file can be simplified quite a bit. This proposal also addresses other comments about proxyNonBids and function naming.
package exchange
import (
"github.com/prebid/openrtb/v20/openrtb3"
"github.com/prebid/prebid-server/v2/exchange/entities"
"github.com/prebid/prebid-server/v2/openrtb_ext"
)
type SeatNonBidBuilder map[string][]openrtb_ext.NonBid
// rejectBid appends a non bid object to the builder based on a bid
func (b SeatNonBidBuilder) rejectBid(bid *entities.PbsOrtbBid, nonBidReason int, seat string) {
if bid == nil || bid.Bid == nil {
return
}
nonBid := openrtb_ext.NonBid{
ImpId: bid.Bid.ImpID,
StatusCode: nonBidReason,
Ext: &openrtb_ext.NonBidExt{
Prebid: openrtb_ext.ExtResponseNonBidPrebid{Bid: openrtb_ext.NonBidObject{
Price: bid.Bid.Price,
ADomain: bid.Bid.ADomain,
CatTax: bid.Bid.CatTax,
Cat: bid.Bid.Cat,
DealID: bid.Bid.DealID,
W: bid.Bid.W,
H: bid.Bid.H,
Dur: bid.Bid.Dur,
MType: bid.Bid.MType,
OriginalBidCPM: bid.OriginalBidCPM,
OriginalBidCur: bid.OriginalBidCur,
}},
},
}
b[seat] = append(b[seat], nonBid)
}
// rejectImps appends a non bid object to the builder for every specified imp
func (b SeatNonBidBuilder) rejectImps(impIds []string, nonBidReason openrtb3.NoBidReason, seat string) {
nonBids := []openrtb_ext.NonBid{}
for _, impId := range impIds {
nonBid := openrtb_ext.NonBid{
ImpId: impId,
StatusCode: int(nonBidReason),
}
nonBids = append(nonBids, nonBid)
}
if len(nonBids) > 0 {
b[seat] = append(b[seat], nonBids...)
}
}
// slice transforms the seat non bid map into a slice of SeatNonBid objects representing the non-bids for each seat
func (b SeatNonBidBuilder) Slice() []openrtb_ext.SeatNonBid {
seatNonBid := make([]openrtb_ext.SeatNonBid, 0)
for seat, nonBids := range b {
seatNonBid = append(seatNonBid, openrtb_ext.SeatNonBid{
Seat: seat,
NonBid: nonBids,
})
}
return seatNonBid
}
Also we shouldn't need to create the seat non bid map from within the receiver functions. We can just create an instance of the builder prior to calling the receiver methods.
…imeout_nonbidreason
…Wrap/prebid-server into i2852_timeout_nonbidreason
@bsardo addressed the review comments, please check. |
seatNonBid = append(seatNonBid, openrtb_ext.SeatNonBid{ | ||
Seat: seat, | ||
NonBid: nonBids, | ||
}) | ||
} | ||
return seatNonBid | ||
} | ||
|
||
// append adds the nonBids from the input nonBids to the current nonBids. | ||
// This method is not thread safe as we are initializing and writing to map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can delete this second comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted.
exchange/non_bid_reason.go
Outdated
// errorCode := errortypes.ReadCode(httpInfo.err) | ||
nonBidReason := errorToNonBidReason(httpInfo.err) | ||
// nonBidReason := errorToNonBidReason(errorCode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented code.
seatBids := make([]*entities.PbsOrtbSeatBid, 0, len(seatBidMap)) | ||
for _, seatBid := range seatBidMap { | ||
seatBids = append(seatBids, seatBid) | ||
} | ||
|
||
extraRespInfo.seatNonBidBuilder = seatNonBidBuilder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this will be nil if there aren't any imps. If there is at least one imp, this will be an empty map or a map with entries if there was at least one http error. Is that correct?
I'm thinking that it might be easier if seatNonBidBuilder
is always not nil. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initialised the seatNonBidBuilder to empty map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add unit tests for rejectImps
and append
functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Unit test for append and rejectImps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you were probably trying to make minimal updates to the code that's here since this part is due to the refactor we asked for but I think we should remove the indirection in these tests as I find it a bit confusing. IMO it is better to be verbose in tests than introduce helper functions like the ones in this file. You can replace what's here with what I provided offline which also covers some cases that are currently missing but that I think we should have:
func TestRejectImps(t *testing.T) {
tests := []struct{
name string
impIDs []string
builder SeatNonBidBuilder
want SeatNonBidBuilder
}{
{
name: "nil_imps",
impIDs: nil,
builder: SeatNonBidBuilder{},
want: SeatNonBidBuilder{},
},
{
name: "empty_imps",
impIDs: []string{},
builder: SeatNonBidBuilder{},
want: SeatNonBidBuilder{},
},
{
name: "one_imp",
impIDs: []string{"imp1"},
builder: SeatNonBidBuilder{},
want: SeatNonBidBuilder{
"seat1": []openrtb_ext.NonBid{
{
ImpId: "imp1",
StatusCode: 300,
},
},
},
},
{
name: "many_imps",
impIDs: []string{"imp1", "imp2"},
builder: SeatNonBidBuilder{},
want: SeatNonBidBuilder{
"seat1": []openrtb_ext.NonBid{
{
ImpId: "imp1",
StatusCode: 300,
},
{
ImpId: "imp2",
StatusCode: 300,
},
},
},
},
{
name: "many_imps_appended_to_prepopulated_list",
impIDs: []string{"imp1", "imp2"},
builder: SeatNonBidBuilder{
"seat0": []openrtb_ext.NonBid{
{
ImpId: "imp0",
StatusCode: 0,
},
},
},
want: SeatNonBidBuilder{
"seat0": []openrtb_ext.NonBid{
{
ImpId: "imp0",
StatusCode: 0,
},
},
"seat1": []openrtb_ext.NonBid{
{
ImpId: "imp1",
StatusCode: 300,
},
{
ImpId: "imp2",
StatusCode: 300,
},
},
},
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
test.builder.rejectImps(test.impIDs, 300, "seat1")
assert.Equal(t, len(test.builder), len(test.want))
for seat := range test.want {
assert.ElementsMatch(t, test.want[seat], test.builder[seat])
}
})
}
}
func TestSlice(t *testing.T) {
tests := []struct{
name string
builder SeatNonBidBuilder
want []openrtb_ext.SeatNonBid
}{
{
name: "nil",
builder: nil,
want: []openrtb_ext.SeatNonBid{},
},
{
name: "empty",
builder: SeatNonBidBuilder{},
want: []openrtb_ext.SeatNonBid{},
},
{
name: "one_no_nonbids",
builder: SeatNonBidBuilder{
"a": []openrtb_ext.NonBid{},
},
want: []openrtb_ext.SeatNonBid{
{
NonBid: []openrtb_ext.NonBid{},
Seat: "a",
},
},
},
{
name: "one_with_nonbids",
builder: SeatNonBidBuilder{
"a": []openrtb_ext.NonBid{
{
ImpId: "imp1",
StatusCode: 100,
},
{
ImpId: "imp2",
StatusCode: 200,
},
},
},
want: []openrtb_ext.SeatNonBid{
{
NonBid: []openrtb_ext.NonBid{
{
ImpId: "imp1",
StatusCode: 100,
},
{
ImpId: "imp2",
StatusCode: 200,
},
},
Seat: "a",
},
},
},
{
name: "many_no_nonbids",
builder: SeatNonBidBuilder{
"a": []openrtb_ext.NonBid{},
"b": []openrtb_ext.NonBid{},
"c": []openrtb_ext.NonBid{},
},
want: []openrtb_ext.SeatNonBid{
{
NonBid: []openrtb_ext.NonBid{},
Seat: "a",
},
{
NonBid: []openrtb_ext.NonBid{},
Seat: "b",
},
{
NonBid: []openrtb_ext.NonBid{},
Seat: "c",
},
},
},
{
name: "many_with_nonbids",
builder: SeatNonBidBuilder{
"a": []openrtb_ext.NonBid{
{
ImpId: "imp1",
StatusCode: 100,
},
{
ImpId: "imp2",
StatusCode: 200,
},
},
"b": []openrtb_ext.NonBid{
{
ImpId: "imp3",
StatusCode: 300,
},
},
"c": []openrtb_ext.NonBid{
{
ImpId: "imp4",
StatusCode: 400,
},
{
ImpId: "imp5",
StatusCode: 500,
},
},
},
want: []openrtb_ext.SeatNonBid{
{
NonBid: []openrtb_ext.NonBid{
{
ImpId: "imp1",
StatusCode: 100,
},
{
ImpId: "imp2",
StatusCode: 200,
},
},
Seat: "a",
},
{
NonBid: []openrtb_ext.NonBid{
{
ImpId: "imp3",
StatusCode: 300,
},
},
Seat: "b",
},
{
NonBid: []openrtb_ext.NonBid{
{
ImpId: "imp4",
StatusCode: 400,
},
{
ImpId: "imp5",
StatusCode: 500,
},
},
Seat: "c",
},
},
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
result := test.builder.Slice()
assert.ElementsMatch(t, test.want, result)
})
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Unit test for append and rejectImps.
if extraRespInfo.seatNonBidBuilder != nil { | ||
seatNonBidBuilder = extraRespInfo.seatNonBidBuilder | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extraRespInfo.seatNonBidBuilder
will never be nil based on how getAllBids
works but perhaps we should keep this nil check given the complexity of this code and since we don't have any unit tests for getAllBids
to ensure it is never nil? @SyntaxNode thoughts?
@bsardo Addressed review comments. Please check. |
@@ -107,3 +78,293 @@ var sampleSeatBids = func(seat string, nonBidCount int) []openrtb_ext.SeatNonBid | |||
seatNonBids = append(seatNonBids, seatNonBid) | |||
return seatNonBids | |||
} | |||
|
|||
func TestAppend(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the following test cases:
{
name: "nil_append",
builder: SeatNonBidBuilder{"seat1": []openrtb_ext.NonBid{{ImpId: "imp1"}}},
toAppend: nil,
expected: SeatNonBidBuilder{"seat1": []openrtb_ext.NonBid{{ImpId: "imp1"}}},
},
{
name: "empty_append",
builder: SeatNonBidBuilder{"seat1": []openrtb_ext.NonBid{{ImpId: "imp1"}}},
toAppend: []SeatNonBidBuilder{},
expected: SeatNonBidBuilder{"seat1": []openrtb_ext.NonBid{{ImpId: "imp1"}}},
},
{
name: "append_multiple_same_seat",
builder: SeatNonBidBuilder{
"seat1": []openrtb_ext.NonBid{
{ImpId: "imp1"},
},
},
toAppend: []SeatNonBidBuilder{
{
"seat1": []openrtb_ext.NonBid{
{ImpId: "imp2"},
},
},
},
expected: SeatNonBidBuilder{
"seat1": []openrtb_ext.NonBid{
{ImpId: "imp1"},
{ImpId: "imp2"},
},
},
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
exchange/seat_non_bids_test.go
Outdated
expected: SeatNonBidBuilder{"seat1": []openrtb_ext.NonBid{{ImpId: "imp1"}}}, | ||
}, | ||
{ | ||
name: "multiple seats", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename to append_one_different_seat
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
exchange/seat_non_bids_test.go
Outdated
expected: SeatNonBidBuilder{"seat1": []openrtb_ext.NonBid{{ImpId: "imp1"}}, "seat2": []openrtb_ext.NonBid{{ImpId: "imp2"}}}, | ||
}, | ||
{ | ||
name: "multiple appends", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename to append_multiple_different_seats
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
exchange/seat_non_bids_test.go
Outdated
expected SeatNonBidBuilder | ||
}{ | ||
{ | ||
name: "nil receiver", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename to nil_builder
to match what t.Run
does automatically with white space substitutions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
exchange/seat_non_bids_test.go
Outdated
expected: nil, | ||
}, | ||
{ | ||
name: "empty builder", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename to empty_builder
to match what t.Run
does automatically with white space substitutions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
}, | ||
}, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the following additional test case:
{
name: "many_imps_appended_to_prepopulated_list_same_seat",
impIDs: []string{"imp1", "imp2"},
builder: SeatNonBidBuilder{
"seat1": []openrtb_ext.NonBid{
{
ImpId: "imp0",
StatusCode: 300,
},
},
},
want: SeatNonBidBuilder{
"seat1": []openrtb_ext.NonBid{
{
ImpId: "imp0",
StatusCode: 300,
},
{
ImpId: "imp1",
StatusCode: 300,
},
{
ImpId: "imp2",
StatusCode: 300,
},
},
},
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
toAppend []SeatNonBidBuilder | ||
expected SeatNonBidBuilder | ||
}{ | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: can you please reformat these test cases to multi-line format similar to how TestRejectImps
is formatted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -9,9 +9,9 @@ import ( | |||
"github.com/stretchr/testify/assert" | |||
) | |||
|
|||
func TestSeatNonBidsAdd(t *testing.T) { | |||
func TestRejectBid(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you ok replacing this test with what I provided here? The version of this test that I provided will remove the level of indirection thus making it easier to follow IMO.
If you agree, please also delete the function variables sampleSeatBids
and sampleSeatNonBidMap
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unit test for RejectBid function, the one you provide is for rejectImps. I have fixed this test and removed sampleSeatBids
and sampleSeatNonBidMap
.
@Pubmatic-Dhruv-Sonone See comments above. Please also resolve the newly introduced merge conflict by merging with master when you get a chance. |
@bsardo Addressed review comments. Please check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Related. to - #2852
@SyntaxNode / @hhhjort : Please review this.
PROPOSAL - Needs adapters to provide impression id associated with the request data
prebid-server/exchange/bidder.go
Line 135 in cc18b8a
requestData
andimpressionId
prebid-server/adapters/bidder.go
Line 120 in cc18b8a