Skip to content

Conversation

@sirknightj
Copy link
Contributor

@sirknightj sirknightj commented Apr 11, 2025

Issue #, if available:

What was changed?

  • Pulling in commits from the develop branch prior to the migration.
  1. When the stream retention is 0, override the content store policy to DROP_TAIL (Add unit test for store pressure policy check #170)
  2. Fixing typos in the readme (Fix section of README describing NativeLibraryPath #174, Add note about demo app expecting stream in us-west-2 #175)
  3. Adding NULL checks when credentialsProvider.getCredentials might return null ([BUG] Null pointer dereference #191)

Why was it changed?

  • Pulling in older commits from the develop branch merge strategy.
  • Change 1: The media is removed from the content store once the persisted acks is received. When retention=0, no persisted acks come.
  • Change 2: Easier for new users
  • Change 3: Handling cases where credentials fail to refresh (e.g. due to no network).
  • Change 4 (new changes, not an older commit):
    • Re-added the constructor which was removed in change 3 (for backwards compatibility).
    • Added more thorough unit tests for the changes in change 1.
    • Added unit tests for the changes in change 3. Made the credentials timeout a constructor parameter so that the test doesn't need to wait 30 seconds for it to timeout.

How was it changed?

  • Merged in older commits.

What testing was done for the changes?

  • Added additional unit tests, and had the CI run it
    • Added a test where credentials refresh timed out -- used 50ms timeout and 3s sleep to simulate a refresh taking a long time.
    • Increasing code coverage

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

niyatim23 and others added 6 commits April 10, 2025 21:24
* add test for store pressure policy check

* set value for storePressurePolicy

* update constructor

* make storePressurePolicy final

* remove additional assignment
* add checks and throw exceptions

* fix ci

* comments

* use log4j2
@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2025

Codecov Report

❌ Patch coverage is 57.14286% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.27%. Comparing base (521e6cd) to head (b34abe7).
⚠️ Report is 33 commits behind head on develop.

Files with missing lines Patch % Lines
...eo/java/service/JavaKinesisVideoServiceClient.java 41.17% 10 Missing ⚠️
.../internal/service/DefaultServiceCallbacksImpl.java 20.00% 3 Missing and 1 partial ⚠️
...rvice/CachedInfoMultiAuthServiceCallbacksImpl.java 20.00% 4 Missing ⚠️
...deo/java/client/KinesisVideoJavaClientFactory.java 0.00% 3 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop     #210      +/-   ##
=============================================
+ Coverage      40.13%   41.27%   +1.13%     
- Complexity       407      426      +19     
=============================================
  Files            103      103              
  Lines           3797     3804       +7     
  Branches         237      242       +5     
=============================================
+ Hits            1524     1570      +46     
+ Misses          2154     2120      -34     
+ Partials         119      114       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sirknightj sirknightj marked this pull request as ready for review April 15, 2025 16:00
Copy link
Contributor

@unicornss unicornss left a comment

Choose a reason for hiding this comment

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

suggest updating the comments to indicate the context for test coverage and shorter runs

@sirknightj
Copy link
Contributor Author

@unicornss, Updated the PR description

@sirknightj sirknightj requested a review from unicornss June 16, 2025 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants