-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Outbrain adapter: Read OB user token from local storage #10382
Outbrain adapter: Read OB user token from local storage #10382
Conversation
@markkuhar please add an associated unit test! |
b1ca3ac
to
4c653e4
Compare
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.
One more nitpick and I promise we are done!
sinon.assert.calledWith(getDataFromLocalStorageStub, 'OB-USER-TOKEN'); | ||
|
||
getDataFromLocalStorageStub.restore(); | ||
}); |
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.
Sorry to be a stickler on this but could you please do the stubbing and restoring in the before and after hooks?
This helps us not hurt downstream tests if for some reason your test fails in the middle of execution.
I know other adapters have it, but trying to set a better standard:
IX has a good example of this: https://github.com/prebid/Prebid.js/blob/master/test/spec/modules/ixBidAdapter_spec.js#L3814-L3833
Or you could use a sandbox if you were stubbing more things, but prob not needed.
Thanks!
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.
hey, thanks for the suggestion! I don't want to stub return of the method before each test because I don't want to fix other tests (the response should stay the same if we don't read anything from local storage). So I hope this is ok
9fd5d42
Hey @robertrmartinez can you please check this PR again? |
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.
Thanks!
Type of change
Bugfix
Feature
New bidder adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other
Description of change
Other information