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.

Sign up for GitHub

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

Jump to bottom

feat(ads): Ignore ad events with no associated ad #4488

Merged
merged 4 commits into from
Sep 21, 2022
Merged

feat(ads): Ignore ad events with no associated ad #4488

merged 4 commits into from
Sep 21, 2022

Conversation

theodab
Copy link
Collaborator

@theodab theodab commented Sep 16, 2022

Issue #4481

@github-actions
Copy link
Contributor

Incremental code coverage: 0.00%

@avelad avelad added this to the v4.3 milestone Sep 16, 2022
@avelad avelad added the component: ads The issue involves the Shaka Player ads API or the use of other ad SDKs label Sep 16, 2022
JulianDomingo
JulianDomingo previously approved these changes Sep 16, 2022
View reviewed changes
littlespex
littlespex requested changes Sep 16, 2022
View reviewed changes
lib/ads/client_side_ad_manager.js Outdated Show resolved Hide resolved
@avelad avelad self-requested a review September 19, 2022 07:45
avelad
avelad previously approved these changes Sep 19, 2022
View reviewed changes
avelad
avelad approved these changes Sep 20, 2022
View reviewed changes
Copy link
Collaborator

@avelad avelad left a comment

Choose a reason for hiding this comment

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

@theodab I think that it's necessary update the PR title to match with the new implementation

@theodab theodab changed the title feat(ads): Add assert for ad events to have ads feat(ads): Ignore ad events with no associated ad Sep 20, 2022
@theodab
Copy link
Collaborator Author

theodab commented Sep 20, 2022

@theodab I think that it's necessary update the PR title to match with the new implementation

Yeah, good point.

@joeyparrish
Copy link
Member

@theodab, could you maybe add a test for the case where there is no ad?

@avelad avelad added the type: enhancement New feature or request label Sep 21, 2022
@avelad avelad merged commit e826eb8 into shaka-project:main Sep 21, 2022
@github-actions github-actions bot mentioned this pull request Sep 21, 2022
Merged
@theodab
Copy link
Collaborator Author

theodab commented Sep 21, 2022

@theodab, could you maybe add a test for the case where there is no ad?

Looking at our ad test, we actually have no tests at all for successfully playing an ad. Only tests for situations where the ads fail to load.
So I'll make a basic test that covers both this case and the success case. As part of another PR, I guess.

@theodab theodab deleted the adAssertBranch branch September 22, 2022 00:37
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@littlespex littlespex littlespex approved these changes

@joeyparrish joeyparrish joeyparrish approved these changes

@avelad avelad avelad approved these changes

@JulianDomingo JulianDomingo Awaiting requested review from JulianDomingo

Assignees

@littlespex littlespex

@joeyparrish joeyparrish

@avelad avelad

@JulianDomingo JulianDomingo

Labels
component: ads The issue involves the Shaka Player ads API or the use of other ad SDKs status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Milestone
v4.3
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@theodab @joeyparrish @littlespex @avelad @JulianDomingo