-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fix vmap mid roll ad position reporting #83
Changes from 5 commits
8e53f90
34ca984
88825f9
0a229a1
6421af1
7ee0bd3
03e0dfe
d91208b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -7,6 +7,10 @@ import com.bitmovin.analytics.conviva.helper.mockLogging | |||||||||||||||||||||||||||||||||||||||||||||||
import com.bitmovin.analytics.conviva.helper.unmockLogging | ||||||||||||||||||||||||||||||||||||||||||||||||
import com.bitmovin.analytics.conviva.ssai.DefaultSsaiApi | ||||||||||||||||||||||||||||||||||||||||||||||||
import com.bitmovin.player.api.Player | ||||||||||||||||||||||||||||||||||||||||||||||||
import com.bitmovin.player.api.advertising.Ad | ||||||||||||||||||||||||||||||||||||||||||||||||
import com.bitmovin.player.api.advertising.AdBreak | ||||||||||||||||||||||||||||||||||||||||||||||||
import com.bitmovin.player.api.advertising.AdData | ||||||||||||||||||||||||||||||||||||||||||||||||
import com.bitmovin.player.api.advertising.AdSourceType | ||||||||||||||||||||||||||||||||||||||||||||||||
import com.bitmovin.player.api.deficiency.PlayerErrorCode | ||||||||||||||||||||||||||||||||||||||||||||||||
import com.bitmovin.player.api.deficiency.PlayerWarningCode | ||||||||||||||||||||||||||||||||||||||||||||||||
import com.bitmovin.player.api.deficiency.SourceWarningCode | ||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -16,6 +20,7 @@ import com.bitmovin.player.api.media.Quality | |||||||||||||||||||||||||||||||||||||||||||||||
import com.bitmovin.player.api.media.video.quality.VideoQuality | ||||||||||||||||||||||||||||||||||||||||||||||||
import com.conviva.sdk.ConvivaAdAnalytics | ||||||||||||||||||||||||||||||||||||||||||||||||
import com.conviva.sdk.ConvivaSdkConstants | ||||||||||||||||||||||||||||||||||||||||||||||||
import com.conviva.sdk.ConvivaSdkConstants.AdPosition | ||||||||||||||||||||||||||||||||||||||||||||||||
import com.conviva.sdk.ConvivaVideoAnalytics | ||||||||||||||||||||||||||||||||||||||||||||||||
import io.mockk.clearMocks | ||||||||||||||||||||||||||||||||||||||||||||||||
import io.mockk.every | ||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -186,6 +191,31 @@ class ConvivaAnalyticsIntegrationTest { | |||||||||||||||||||||||||||||||||||||||||||||||
verify(exactly = 0) { videoAnalytics.reportPlaybackError(any(), any()) } | ||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
@Test | ||||||||||||||||||||||||||||||||||||||||||||||||
fun `reports CSAI ad position based on last ad break schedule time`() { | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
strangesource marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||
player.listeners[PlayerEvent.AdBreakStarted::class]?.forEach { | ||||||||||||||||||||||||||||||||||||||||||||||||
it(createAdBreakStartedEvent(10.0)) | ||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
verify { videoAnalytics.reportAdBreakStarted(any(), any()) } | ||||||||||||||||||||||||||||||||||||||||||||||||
player.listeners[PlayerEvent.AdStarted::class]?.forEach { it(TEST_AD_STARTED_EVENT) } | ||||||||||||||||||||||||||||||||||||||||||||||||
verify { | ||||||||||||||||||||||||||||||||||||||||||||||||
adAnalytics.reportAdStarted(match { it["c3.ad.position"] == AdPosition.MIDROLL }) | ||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
player.listeners[PlayerEvent.AdBreakStarted::class]?.forEach { | ||||||||||||||||||||||||||||||||||||||||||||||||
it(createAdBreakStartedEvent(00.0)) | ||||||||||||||||||||||||||||||||||||||||||||||||
strangesource marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
verify { videoAnalytics.reportAdBreakStarted(any(), any()) } | ||||||||||||||||||||||||||||||||||||||||||||||||
player.listeners[PlayerEvent.AdStarted::class]?.forEach { it(TEST_AD_STARTED_EVENT) } | ||||||||||||||||||||||||||||||||||||||||||||||||
verify { | ||||||||||||||||||||||||||||||||||||||||||||||||
adAnalytics.reportAdStarted(match { it["c3.ad.position"] == AdPosition.PREROLL }) | ||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't know this |
||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
companion object { | ||||||||||||||||||||||||||||||||||||||||||||||||
@JvmStatic | ||||||||||||||||||||||||||||||||||||||||||||||||
@BeforeClass | ||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -235,3 +265,49 @@ private val attachedPlayerEvents = listOf( | |||||||||||||||||||||||||||||||||||||||||||||||
SourceEvent.Error::class, | ||||||||||||||||||||||||||||||||||||||||||||||||
SourceEvent.Warning::class, | ||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
private val TEST_AD_STARTED_EVENT = PlayerEvent.AdStarted( | ||||||||||||||||||||||||||||||||||||||||||||||||
clientType = AdSourceType.Ima, | ||||||||||||||||||||||||||||||||||||||||||||||||
clickThroughUrl = "clickThroughUrl", | ||||||||||||||||||||||||||||||||||||||||||||||||
duration = 10.0, | ||||||||||||||||||||||||||||||||||||||||||||||||
timeOffset = 10.0, | ||||||||||||||||||||||||||||||||||||||||||||||||
position = "0.0", | ||||||||||||||||||||||||||||||||||||||||||||||||
skipOffset = 10.0, | ||||||||||||||||||||||||||||||||||||||||||||||||
ad = object : Ad { | ||||||||||||||||||||||||||||||||||||||||||||||||
override val clickThroughUrl: String? | ||||||||||||||||||||||||||||||||||||||||||||||||
get() = "clickThroughUrl" | ||||||||||||||||||||||||||||||||||||||||||||||||
override val data: AdData? | ||||||||||||||||||||||||||||||||||||||||||||||||
get() = null | ||||||||||||||||||||||||||||||||||||||||||||||||
override val height: Int | ||||||||||||||||||||||||||||||||||||||||||||||||
get() = 100 | ||||||||||||||||||||||||||||||||||||||||||||||||
override val id: String? | ||||||||||||||||||||||||||||||||||||||||||||||||
get() = null | ||||||||||||||||||||||||||||||||||||||||||||||||
override val isLinear: Boolean | ||||||||||||||||||||||||||||||||||||||||||||||||
get() = true | ||||||||||||||||||||||||||||||||||||||||||||||||
override val mediaFileUrl: String? | ||||||||||||||||||||||||||||||||||||||||||||||||
get() = null | ||||||||||||||||||||||||||||||||||||||||||||||||
override val width: Int | ||||||||||||||||||||||||||||||||||||||||||||||||
get() = 200 | ||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Super nit. Why implement this though getters rather than fields?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 7ee0bd3 I went for a mock to make it easier to read. |
||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
override fun clickThroughUrlOpened() {} | ||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||
indexInQueue = 0, | ||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
private fun createAdBreakStartedEvent(scheduleTime: Double): PlayerEvent.AdBreakStarted { | ||||||||||||||||||||||||||||||||||||||||||||||||
val adBreakStarted = PlayerEvent.AdBreakStarted( | ||||||||||||||||||||||||||||||||||||||||||||||||
adBreak = object : AdBreak { | ||||||||||||||||||||||||||||||||||||||||||||||||
override val ads: List<Ad> | ||||||||||||||||||||||||||||||||||||||||||||||||
get() = emptyList() | ||||||||||||||||||||||||||||||||||||||||||||||||
override val id: String | ||||||||||||||||||||||||||||||||||||||||||||||||
get() = "" | ||||||||||||||||||||||||||||||||||||||||||||||||
override val replaceContentDuration: Double? | ||||||||||||||||||||||||||||||||||||||||||||||||
get() = null | ||||||||||||||||||||||||||||||||||||||||||||||||
override val scheduleTime: Double | ||||||||||||||||||||||||||||||||||||||||||||||||
get() { | ||||||||||||||||||||||||||||||||||||||||||||||||
return scheduleTime | ||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||
return adBreakStarted | ||||||||||||||||||||||||||||||||||||||||||||||||
} |
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.
shouldn't
callAndExpectEvent
be used instead? The AdBreak start could be missed.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.
Yes, in theory this could be cary. Unfortunately the test helpers seem to have an issue that cause a deadlock when loading in the callAndExpectEvent. 🙈 I'll keep it as-is. We should have a look at the test helpers at some point.