awslabs / amazon-kinesis-video-streams-pic

Apache License 2.0
49 stars 47 forks source link

Fix readFile on Windows #249

Closed niyatim23 closed 6 months ago

niyatim23 commented 6 months ago

Issue #, if available:

What was changed?

Why was it changed?

How was it changed?

What testing was done for the changes?

Screenshot 2024-02-22 at 8 54 11 AM

Screenshot 2024-02-22 at 8 53 52 AM

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

codecov-commenter commented 6 months ago

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 80.41%. Comparing base (ffef12d) to head (97c2548). Report is 1 commits behind head on develop.

:exclamation: Current head 97c2548 differs from pull request most recent head 6b63034. Consider uploading reports for the commit 6b63034 to get more accurate results

Files Patch % Lines
src/utils/src/FileIo.c 40.00% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #249 +/- ## =========================================== + Coverage 80.34% 80.41% +0.06% =========================================== Files 53 53 Lines 10808 10810 +2 =========================================== + Hits 8684 8693 +9 + Misses 2124 2117 -7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

hassanctech commented 6 months ago

Nice! Let's add some unit test coverage here for this bug fix, one that fails before the change but succeeds now.

disa6302 commented 6 months ago

Awesome! In addition to unit test, are there unit tests in WebRTC we should remove from the gtest filter to test file related operations (I think the file caching tests in there are disabled at the moment for windows) ? If so, can we open a PR in webrtc referencing this commit, enabling the file cache specific tests to make sure it is all good?

niyatim23 commented 6 months ago

Awesome! In addition to unit test, are there unit tests in WebRTC we should remove from the gtest filter to test file related operations (I think the file caching tests in there are disabled at the moment for windows) ? If so, can we open a PR in webrtc referencing this commit, enabling the file cache specific tests to make sure it is all good?

Enabled Signaling tests on Windows in: https://github.com/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/pull/1933