Skip to content

Fix MonitoringTest by using PillarboxExoPlayer directly #714

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
Merged
StaehliJ merged 5 commits into from
Sep 18, 2024
Merged

Fix MonitoringTest by using PillarboxExoPlayer directly #714

StaehliJ merged 5 commits into from
Sep 18, 2024

Conversation

@MGaetan89
Copy link
Contributor

@MGaetan89 MGaetan89 commented Sep 18, 2024 •
edited
Loading

Pull request

Description

The MonitoringTest was creating its own Monitoring instance to try to replicate PillarboxExoPlayer internals. This PR changes that to reuse the instance from PillarboxExoPlayer directly.
I've also added a TestDispatcher to be used in PillarboxExoPlayer instead of EmptyCoroutineContext, so the heartbeat task is more tied to the test being run.

Changes made

  • Use Monitoring from PillarboxExoPlayer in MonitoringTest.
  • Use a dedicated TestDispatcher in MonitoringTest.
  • Clear mocks and player in ComScoreTrackerIntegrationTest.
  • Add missing Looper definition CommandersActStreamingTest.

Checklist

  • Your branch has been rebased onto the main branch.
  • APIs have been properly documented (if relevant).
  • The documentation has been updated (if relevant).
  • New unit tests have been written (if relevant).
  • The demo has been updated (if relevant).
  • All pull request status checks pass.

@MGaetan89 MGaetan89 self-assigned this Sep 18, 2024
@MGaetan89 MGaetan89 added this to the Test milestone Sep 18, 2024
@github-actions
Copy link

github-actions bot commented Sep 18, 2024 •
edited
Loading

Code Coverage

Overall Project 49.82% 🟢
Files changed 100% 🟢

Module Coverage
:pillarbox-core-business 80.38% 🟢
:pillarbox-player 59.7% 🟢
Files
Module File Coverage
:pillarbox-core-business CommandersActStreaming.kt 95.27% 🟢
:pillarbox-player PillarboxExoPlayer.kt 81.48% 🟢

@MGaetan89 MGaetan89 requested a review from StaehliJ September 18, 2024 06:48
@MGaetan89 MGaetan89 marked this pull request as ready for review September 18, 2024 06:48
@MGaetan89 MGaetan89 marked this pull request as draft September 18, 2024 06:54
@MGaetan89
Copy link
Contributor Author

MGaetan89 commented Sep 18, 2024

It looks like I was lucky the first couple of times I ran the tests on the CI 😅
I'll keep investigating this flakiness.

@MGaetan89 MGaetan89 marked this pull request as ready for review September 18, 2024 07:43
@MGaetan89 MGaetan89 force-pushed the fix_MonitoringTest_flakiness branch from ef6d0df to 7d4225a Compare September 18, 2024 09:17
@MGaetan89
Copy link
Contributor Author

MGaetan89 commented Sep 18, 2024

It should be good for review now @StaehliJ

@StaehliJ StaehliJ added this pull request to the merge queue Sep 18, 2024
Merged via the queue into main with commit 6d7053c Sep 18, 2024
@StaehliJ StaehliJ deleted the fix_MonitoringTest_flakiness branch September 18, 2024 11:42
MGaetan89 added a commit that referenced this pull request Sep 18, 2024
@MGaetan89
Fix MonitoringTest by using PillarboxExoPlayer directly (#714)
267ca0c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StaehliJ StaehliJ StaehliJ approved these changes

Assignees

@MGaetan89 MGaetan89

Labels

None yet

Projects

Pillarbox
Archived in project

Milestone

Test

Development

Successfully merging this pull request may close these issues.

2 participants

@MGaetan89 @StaehliJ