-
-
Notifications
You must be signed in to change notification settings - Fork 363
refactor(IVideoDevice): add bool result from open method #5945
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
Conversation
Reviewer's GuideThis pull request modifies the File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
- The
flipcamera functionality was removed; consider mentioning this significant change in the PR description.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| await service.Flip(); | ||
| var url = await service.GetPreviewUrl(); | ||
| Assert.Equal("blob:https://test-preview", url); | ||
| } |
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 (testing): Add test case for open method failure
Add a test for when JS navigator.mediaDevices.getUserMedia fails, verifying that the C# Open method returns false on interop failure.
Suggested implementation:
Assert.Equal("blob:https://test-preview", url);
}
[Fact]
public async Task Open_ReturnsFalse_WhenUserMediaFails()
{
// Arrange
var jsRuntime = new FailingMockJSRuntime();
var service = new VideoDeviceService(jsRuntime);
// Act
bool result = await service.Open(".bb-video");
// Assert
Assert.False(result);
}
// Failing mock that simulates navigator.mediaDevices.getUserMedia failure.
private class FailingMockJSRuntime : IJSRuntime
{
public Task<TValue> InvokeAsync<TValue>(string identifier, object[] args)
{
return Task.FromException<TValue>(new Exception("navigator.mediaDevices.getUserMedia error"));
}
public Task<TValue> InvokeAsync<TValue>(string identifier, CancellationToken cancellationToken, object[] args)
{
return Task.FromException<TValue>(new Exception("navigator.mediaDevices.getUserMedia error"));
}
}Make sure that:
- The test framework (xUnit) and required namespaces (e.g., using System.Threading, using System.Threading.Tasks, using Microsoft.JSInterop, and using Xunit) are referenced at the top of the file.
- The VideoDeviceService class has a constructor that accepts an IJSRuntime, and its Open method internally catches the JS interop failure and returns false.
- Adjust the details if your codebase implements JS interop differently.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5945 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 669 669
Lines 30551 30544 -7
Branches 4347 4347
=========================================
- Hits 30551 30544 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Link issues
fixes #5944
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Refactor the video device open method to return a boolean result indicating success or failure of opening the media device
Bug Fixes:
Enhancements:
Chores: