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

New Adapter: Alkimi #3247

Merged
merged 15 commits into from
Nov 20, 2023
Merged

New Adapter: Alkimi #3247

merged 15 commits into from
Nov 20, 2023

Conversation

kalidas-alkimi
Copy link
Contributor

Alkimi adapter already supported on prebid-server-java repo, now added on go repo

adapters/alkimi/alkimi.go Outdated Show resolved Hide resolved
@bsardo bsardo changed the title Added Alkimi adapter on PBS-GO New Adapter: Alkimi Oct 23, 2023
@prebid prebid deleted a comment from github-actions bot Oct 26, 2023
@prebid prebid deleted a comment from github-actions bot Oct 26, 2023
@prebid prebid deleted a comment from github-actions bot Oct 26, 2023
assert.NotNil(t, bidder)
}

func TestMakeRequests(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed

@kalidas-alkimi could you link the added SON framework tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope this is also a recommendation, we will update it in future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hope this is also a recommendation, we will update it in future.

PBS-GO expects to add json framework tests whenever new adapter gets added

Copy link

Choose a reason for hiding this comment

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

We don't fully understand, JSON tests are required or not? Should we re-write our unit tests or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

I hope this is also a recommendation, we will update it in future.

PBS-GO expects to add json framework tests whenever new adapter gets added

@pro-nsk JSON tests are required

Copy link

Choose a reason for hiding this comment

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

@onkarvhanumante Added JSON tests

Copy link
Contributor

@onkarvhanumante onkarvhanumante Nov 16, 2023

Choose a reason for hiding this comment

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

@onkarvhanumante Added JSON tests

@pro-nsk JSON test is expected to cover code path of MakeRequests and MakeBids. Now that JSON tests are added, could remove following tests.

TestMakeRequests, TestMakeBidsShouldReturnErrorIfResponseBodyContainsIncorrectImp, TestMakeBidsShouldReturnErrorIfResponseBodyContainsIncorrectImp,
TestMakeBidsShouldReturnEmptyListIfBidResponseIsNull,
TestMakeBidsShouldReturnEmptyListIfBidResponseIsError,
TestMakeBidsShouldReturnBidWithResolvedMacros,
TestMakeBidsShouldReturnErrorIfResponseBodyContainsIncorrectImp,
TestMakeBidsShouldReturnEmptyListIfBidResponseIsNull,
TestMakeBidsShouldReturnEmptyListIfBidResponseIsError,
TestMakeBidsShouldReturnBidWithResolvedMacros,
TestMakeBidsShouldReturnBidForAllTypes

Copy link

Choose a reason for hiding this comment

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

@github-actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, b0a394c

alkimi

Refer here for heat map coverage report

github.com/prebid/prebid-server/adapters/alkimi/alkimi.go:29:	Builder			100.0%
github.com/prebid/prebid-server/adapters/alkimi/alkimi.go:42:	MakeRequests		87.5%
github.com/prebid/prebid-server/adapters/alkimi/alkimi.go:55:	_updateImps		91.3%
github.com/prebid/prebid-server/adapters/alkimi/alkimi.go:93:	_buildBidderRequest	100.0%
github.com/prebid/prebid-server/adapters/alkimi/alkimi.go:108:	MakeBids		90.5%
github.com/prebid/prebid-server/adapters/alkimi/alkimi.go:144:	_resolveMacros		100.0%
github.com/prebid/prebid-server/adapters/alkimi/alkimi.go:151:	_getMediaTypeForImp	36.4%
total:								(statements)		84.4%

Copy link
Contributor

@onkarvhanumante onkarvhanumante left a comment

Choose a reason for hiding this comment

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

few changes

Copy link

github-actions bot commented Nov 9, 2023

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 87a2333

alkimi

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:30:	Builder			100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:43:	MakeRequests		81.8%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:62:	updateImps		78.6%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:108:	buildBidderRequest	100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:123:	MakeBids		92.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:168:	resolveMacros		100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:174:	getMediaTypeForImp	100.0%
total:									(statements)		88.6%

@kalidas-alkimi
Copy link
Contributor Author

@kalidas-alkimi

PR tests are failing with following error

Run ./validate.sh --nofmt --cov --race 10
gofmt needs to be run, 1 files have issues.  Below is a list of files to review:
adapters/alkimi/alkimi.go

issue seems related to formating in alkimi.go should run gofmt to reformat alkimi.go

Fixed

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, f970b00

alkimi

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:30:	Builder			100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:43:	MakeRequests		81.8%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:62:	updateImps		78.6%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:108:	buildBidderRequest	100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:123:	MakeBids		92.3%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:169:	resolveMacros		100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:175:	getMediaTypeForImp	100.0%
total:									(statements)		88.5%

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, c39d1cb

alkimi

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:30:	Builder			100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:43:	MakeRequests		81.8%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:62:	updateImps		78.6%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:108:	buildBidderRequest	100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:123:	MakeBids		96.2%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:169:	resolveMacros		100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:175:	getMediaTypeForImp	100.0%
total:									(statements)		89.7%

@onkarvhanumante
Copy link
Contributor

@kalidas-alkimi @pro-nsk

PR checks are failing with following error. Run gofmt command to fix tests

gofmt needs to be run, 1 files have issues.  Below is a list of files to review:
adapters/alkimi/alkimi_test.go
Error: Process completed with exit code 1.

@pro-nsk
Copy link

pro-nsk commented Nov 16, 2023

@kalidas-alkimi @pro-nsk

PR checks are failing with following error. Run gofmt command to fix tests

gofmt needs to be run, 1 files have issues.  Below is a list of files to review:
adapters/alkimi/alkimi_test.go
Error: Process completed with exit code 1.

fixed

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 46a764d

alkimi

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:30:	Builder			100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:43:	MakeRequests		81.8%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:62:	updateImps		78.6%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:108:	buildBidderRequest	100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:123:	MakeBids		96.2%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:169:	resolveMacros		100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:175:	getMediaTypeForImp	100.0%
total:									(statements)		89.7%

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, c3fe4cc

alkimi

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:30:	Builder			100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:43:	MakeRequests		81.8%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:62:	updateImps		75.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:108:	buildBidderRequest	100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:123:	MakeBids		96.2%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:169:	resolveMacros		100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:175:	getMediaTypeForImp	100.0%
total:									(statements)		88.5%

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 4541d7e

alkimi

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:30:	Builder			100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:43:	MakeRequests		81.8%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:62:	updateImps		78.6%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:108:	buildBidderRequest	100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:123:	MakeBids		96.2%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:169:	resolveMacros		100.0%
github.com/prebid/prebid-server/v2/adapters/alkimi/alkimi.go:175:	getMediaTypeForImp	100.0%
total:									(statements)		89.7%

Comment on lines +2 to +3
maintainer:
email: [email protected]
Copy link
Contributor

@onkarvhanumante onkarvhanumante Nov 20, 2023

Choose a reason for hiding this comment

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

@kalidas-alkimi @pro-nsk an email is sent from prebid team to verify above mentioned email address.

Requesting to respond back "received" message

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

@onkarvhanumante onkarvhanumante merged commit 06e1145 into prebid:master Nov 20, 2023
5 checks passed
svamiftah pushed a commit to sovrn/prebid-server that referenced this pull request Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants