-
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: suppress some ad events when outside of an ad break #102
fix: suppress some ad events when outside of an ad break #102
Conversation
…sitions also check for adplay and adplaying filtering after ads have finished playing
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.
On a quick glance only nits and minor suggestions. Otherwise, LGTM!
library/src/main/java/com/mux/stats/sdk/muxstats/MuxStatsSdkMedia3.kt
Outdated
Show resolved
Hide resolved
eventBus.dispatch(event) | ||
if (muxPlayerState == MuxPlayerState.PLAYING_ADS) { | ||
eventBus.dispatch(event) | ||
} else { |
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.
nitpick: else if
vs. else
+ (nested) if
?
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.
adCollector.dispatch(AdPlayEvent(null)) | ||
adCollector.dispatch(AdPlayingEvent(null)) | ||
|
||
Assert.assertTrue( |
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.
suggestion(non-blocking): use a loop + all expected filtered out ad events for your assertions. Alternatively, just write individual tests for every ad event like you did for the "allowed" ones, below (which allows test to better isolate an issue)
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.
👍
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.
me three
Filter out events we do not expect to receive outside of an ad break.