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

add getUserSyncs function in clickforceBidAdapter #2383

Merged
merged 6 commits into from
Apr 27, 2018
Merged

add getUserSyncs function in clickforceBidAdapter #2383

merged 6 commits into from
Apr 27, 2018

Conversation

MIGOdanis
Copy link
Contributor

Type of change

  • Feature

Description of change

Add getUserSyncs function in the adapters

@mike-chowla
Copy link
Contributor

Code coverage on this adapter is zero because it has no tests!

There's a couple of other issues I see:
(1) Responses are not coming back gzipped
(2) The test parameters from the original adapter submission are not returning an ad

@MIGOdanis
Copy link
Contributor Author

Hi @mike-chowla , Sorry about that

(1) Responses are not coming back gzipped

We have fix this issues

(2) The test parameters from the original adapter submission are not returning an ad

This issues is the demo camapign is expired. I will create a new campaign,you can get the ad response now.

Thank you.

@mike-chowla
Copy link
Contributor

The code itself looks fine but since there are no tests, it doesn't meet the 80% coverage requirement. The initial adapter PR should have been rejected because there are no tests.

@MIGOdanis
Copy link
Contributor Author

The issues is begin our adserver update new version. so, at initial adapter PR test is work.
What's next we need do?

@mike-chowla
Copy link
Contributor

There should be a unit file that gets to at least 80% code coverage on your adapter.

Here's an example:
https://github.com/prebid/Prebid.js/blob/master/test/spec/modules/appnexusBidAdapter_spec.js

@MIGOdanis
Copy link
Contributor Author

MIGOdanis commented Apr 18, 2018

HI @mike-chowla

we have a unit file now, Please check it.

Thanks

@mike-chowla
Copy link
Contributor

Thanks for adding the test coverage file.

You are just shy of the 80% mark. If you add any test for getUserSyncs(), you should get there.

Also, I never saw the updated test parameters.

If you can get these to me tomorrow (not too late Pacific time), I'll take another pass and we should be able to get you into version 1.9

@MIGOdanis
Copy link
Contributor Author

@mike-chowla
so, we need add test getUserSyncs in the test coverage file?

@mike-chowla
Copy link
Contributor

Yes, it's the only function missing coverage and adding one should get you above the 80% threshold

@MIGOdanis
Copy link
Contributor Author

@mike-chowla

The test parameters already updated. And I have add a getUserSyncs test in the test coverage file. Please check it.

Thank you very much

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.

LGTM.

Code coverage is now 100%!

@mike-chowla
Copy link
Contributor

A couple of comments on items that would make life easier for reviewers:

Your Access-Control-Allow-Origin only works on default ports. When I test on a separate port, it fails:
screen shot 2018-04-27 at 2 04 10 pm

The cpm on your test ad is 0. Prebid doesn't select ads with zero CPM.

@mike-chowla mike-chowla merged commit 1ed012a into prebid:master Apr 27, 2018
@MIGOdanis
Copy link
Contributor Author

MIGOdanis commented Apr 28, 2018

ok. I will change a campaign for the test ad. thank you very much. and the Access-Control-Allow-Origin I will fix that. before next monday

dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* add getUserSyncs function

* bug fix

* add test spec

* fix of docment format

* fix of end line

* add getUserSyncs function into test file and updated test parameters
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.

2 participants