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

Dailymotion bid adapter: add ortb converter and floor price support #12784

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kvnsw
Copy link
Contributor

@kvnsw kvnsw commented Feb 18, 2025

Type of change

Description of change

Added support of oRTB & floor prices for Dailymotion Bid Adapter

@ChrisHuie
Copy link
Collaborator

@kvnsw looks like your unit tests are failing on this pr in one of the browsers on circleci

@kvnsw
Copy link
Contributor Author

kvnsw commented Feb 19, 2025

Hey @ChrisHuie, thanks, I thought it was unrelated to our changes (considering the tests that are failing).

I locally dropped our commit and re-ran the tests and am getting the same kind of errors 🤔 (see screenshot, git log on the right section). Is there an issue somewhere else?

Capture d’écran 2025-02-19 à 18 12 34

Edit: Looks like other contributions are having the same issues (eg: #12785)

@patmmccann
Copy link
Collaborator

@kvnsw your test failures are due to your changes and are unrelated to the iiq pr

@kvnsw kvnsw force-pushed the master branch 2 times, most recently from 2ed9c41 to ab07eb1 Compare February 20, 2025 16:48
Comment on lines 2147 to 2161
config.setConfig({
userSync: {
syncEnabled: true,
filterSettings: {
image: {
bidders: ['dailymotion'],
filter: 'include'
},
iframe: {
bidders: ['dailymotion'],
filter: 'exclude',
},
}
}
});
Copy link
Collaborator

@dgirardi dgirardi Feb 20, 2025

Choose a reason for hiding this comment

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

this should be in a setup block (before or beforeEach) and torn down with config.resetConfig().

As it is here (outside of any test case) it's executed before any test, so unless you run your suite in isolation it will interfere with tests that do not expect it, and by the time your tests run they won't necessarily see this configuration.

@kvnsw kvnsw force-pushed the master branch 2 times, most recently from 9a57531 to 64cb2dc Compare February 21, 2025 10:30
@kvnsw
Copy link
Contributor Author

kvnsw commented Feb 21, 2025

Hello @dgirardi, @patmmccann, @ChrisHuie thank you for your reviews!
We should be good now :)

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.

5 participants