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 user sync pixel logic in Adtelligent adapter #3359

Merged
merged 5 commits into from
Dec 10, 2018

Conversation

GeneGenie
Copy link
Contributor

@GeneGenie GeneGenie commented Dec 7, 2018

  • Feature

Description of change

Adding cookie sync pixel support for Adtelligent adapter
Adding gdpr data support

Related PRs:

Misc

@jsnellbaker jsnellbaker self-assigned this Dec 7, 2018
@jsnellbaker jsnellbaker self-requested a review December 7, 2018 15:58
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

@GeneGenie The new code looks good.

However can you please put together some unit tests to support this new code? With this addition, the code-coverage for the adapter file is at 70%; it should be at least 80%.

We should be good once this is added.

@GeneGenie
Copy link
Contributor Author

GeneGenie commented Dec 7, 2018

@jsnellbaker I would be glad to do this, but
gulp test
and
gulp serve

are failing with this trail :(

[20:38:20] Working directory changed to ~/work/Prebid.js
[20:38:21] Using gulpfile ~/work/Prebid.js/gulpfile.js
/usr/local/lib/node_modules/gulp/bin/gulp.js:129
    gulpInst.start.apply(gulpInst, toRun);
                   ^

TypeError: Cannot read property 'apply' of undefined
    at /usr/local/lib/node_modules/gulp/bin/gulp.js:129:20
    at process._tickCallback (internal/process/next_tick.js:150:11)
    at Function.Module.runMain (module.js:703:11)
    at startup (bootstrap_node.js:193:16)
    at bootstrap_node.js:617:3

BUT! npm test runs good =\ but takes too much time

UP: downgraded global gulp and it works now. But --file option is not applied for some reason
UP: made it work with file, but it fails with unexpected import statement =\

@jsnellbaker
Copy link
Collaborator

@GeneGenie Did you install/setup gulp-cli? There's a note about it on the install part of the README, here.

@jsnellbaker
Copy link
Collaborator

Just saw your updates; can you copy the full import error so I can see?

@GeneGenie
Copy link
Contributor Author

@jsnellbaker

 gulp test --file ./test/spec/modules/adtelligentBidAdapter_spec.js 
[21:03:55] Using gulpfile ~/work/Prebid.js/gulpfile.js
[21:03:55] Starting 'test'...
[21:03:55] Starting 'clean'...
[21:03:55] Finished 'clean' after 57 ms
[21:03:55] Starting 'lint'...
[21:04:13] Finished 'lint' after 17 s
[21:04:13] Starting 'test'...

START:
07 12 2018 21:04:13.444:INFO [karma]: Karma v2.0.5 server started at http://0.0.0.0:9876/
07 12 2018 21:04:13.446:INFO [launcher]: Launching browser ChromeHeadless with unlimited concurrency
07 12 2018 21:04:13.452:INFO [launcher]: Starting browser ChromeHeadless
07 12 2018 21:04:13.814:INFO [HeadlessChrome 0.0.0 (Mac OS X 10.11.6)]: Connected on socket E9biGb-51DzvPabVAAAA with id 62387498
HeadlessChrome 0.0.0 (Mac OS X 10.11.6) ERROR
  {
    "message": "Uncaught SyntaxError: Unexpected token {\nat test/spec/modules/adtelligentBidAdapter_spec.js:1:8\n\nSyntaxError: Unexpected token {",
    "str": "Uncaught SyntaxError: Unexpected token {\nat test/spec/modules/adtelligentBidAdapter_spec.js:1:8\n\nSyntaxError: Unexpected token {"
  }

Finished in 0.144 secs / 0 secs @ 21:04:13 GMT+0200 (EET)

SUMMARY:
✔ 0 tests completed
[21:04:13] 'test' errored after 977 ms
[21:04:13] Error: Karma tests failed with exit code 1
    at /Users/moroz/work/Prebid.js/gulpfile.js:229:12
    at removeAllListeners (/Users/moroz/work/Prebid.js/node_modules/karma/lib/server.js:334:9)
    at webServer.close (/Users/moroz/work/Prebid.js/node_modules/karma/lib/server.js:344:11)
    at Server.close (net.js:1625:9)
    at Object.onceWrapper (events.js:255:19)
    at Server.emit (events.js:165:20)
    at emitCloseNT (net.js:1676:8)
    at process._tickCallback (internal/process/next_tick.js:152:19)
[21:04:13] 'test' errored after 18 s

Where adtelligentBidAdapter_spec.js:1 is:
import {expect} from 'chai';

@jsnellbaker
Copy link
Collaborator

@GeneGenie Thanks for providing the output; this type command isn't possible anymore due to the karma-webpack plugin not supporting the testing of individual files.

You'll either have to run the full gulp test command or carefully use the .only property in your unit test file. If you insert the reference like the following describe.only('adtelligentBidAdapter', function () {, when you run gulp test - it will only run the tests for your adapter.

You'll have to remove this reference when you want to commit the changes, but it can be used while you're putting the tests together.

@GeneGenie
Copy link
Contributor Author

Thank you for help!

@GeneGenie
Copy link
Contributor Author

@jsnellbaker Tests were added to get through min required lvl =)
Thanks for help again

@jsnellbaker jsnellbaker merged commit f424cd2 into prebid:master Dec 10, 2018
loic-talon pushed a commit to onfocusio/Prebid.js that referenced this pull request Dec 19, 2018
* Add user sync pixel logic in Adtelligent adapter

* Add gdpr support

* Add gdpr support

* Update tests
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jan 30, 2019
* Add user sync pixel logic in Adtelligent adapter

* Add gdpr support

* Add gdpr support

* Update tests
pedrolopezmrf pushed a commit to Marfeel/Prebid.js that referenced this pull request Mar 18, 2019
* Add user sync pixel logic in Adtelligent adapter

* Add gdpr support

* Add gdpr support

* Update tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants