feat: Update Media Content Time Spent Calculations with Ad Breaks#74
Conversation
|
Mansi-mParticle
requested review from
BrandonStalnaker,
jamesnrokt,
nickolas-dimitrakas,
rmi22186 and
thomson-t
jamesnrokt
left a comment
jamesnrokt
left a comment
There was a problem hiding this comment.
Ran tests locally as there's no test steps on this repo, would be good to add as a verification step but if time pressured then LGTM
| mediaSession.logAdBreakStart { | ||
| id = "break-2" | ||
| } | ||
| Thread.sleep(1000) // should count toward content time when flag disabled |
There was a problem hiding this comment.
this should be enabled since it's true
There was a problem hiding this comment.
why didn't this comment stop the PR? this test is indeed asserting the opposite of what it should be.
There was a problem hiding this comment.
I actually added this comment after it was merged.
| } | ||
|
|
||
| private fun pauseContentTimeIfAdBreakExclusionEnabled() { | ||
| if (!excludeAdBreaksFromContentTime) { |
| streamType = builder.streamType.require("streamType") | ||
| logMPEvents = builder.logMPEvents | ||
| logMediaEvents = builder.logMediaEvents | ||
| this.excludeAdBreaksFromContentTime = builder.excludeAdBreaksFromContentTime |
There was a problem hiding this comment.
why do you have a different public and private way of describing the same thing? you use the word "exclude" here and "pause" in the public. that leads to confusion and perhaps even led to the bug noted below?
| mediaSession.logPause() | ||
|
|
||
| val contentTime = mediaSession.mediaContentTimeSpent | ||
| assertEquals(2.0, contentTime) |
There was a problem hiding this comment.
this and any other asserts are going to be flakey. you can't guarantee the code is going to run for exactly this long.
| mediaSession.logAdBreakStart { | ||
| id = "break-2" | ||
| } | ||
| Thread.sleep(1000) // should count toward content time when flag disabled |
There was a problem hiding this comment.
why didn't this comment stop the PR? this test is indeed asserting the opposite of what it should be.
## [1.7.0](v1.6.0...v1.7.0) (2025-12-12) ### Features * Update Media Content Time Spent Calculations with Ad Breaks ([#74](#74)) ([c455557](c455557)) ### Bug Fixes * adjust ad break exclusion logic ([#76](#76)) ([84b1493](84b1493)) ### Updates & Maintenance * Migrate from OSSRH to Central Publishing Portal ([#75](#75)) ([ac8de37](ac8de37))
|
🎉 This PR is included in version 1.7.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
6 participants
Instructions
Summary
Testing Plan
Reference Issue