Skip to content

Support DI and Mocking better. #633

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

Merged
merged 1 commit into from
Apr 6, 2022

Conversation

danatcofo
Copy link
Contributor

Convert main static references off of Electron into interface implementation and expose the underlying Socket for low level interaction.

This is a utility PR to improve support for testing and also better support standard DI practices. Additionally included is exposure of the underlying Socket vie the ApplicationSocket class so that we can support more low level socket interactions without having to update the ElectronNET repo directly.

Note: I applied all the comment documentation to the interfaces but did not convert origination to use <inheritdoc /> as I felt that would be inappropriate without some feedback.

…mentation and expose the underlying Socket for low level interaction.
@niklasstich
Copy link

Hey,
first of all, thanks very much for your work on this pull request, this makes testing components that use the Electron.NET API so much easier and I hope it can be merged soon!

I still have a question about mocking though: I tried to make a test for a component in my example project which saves a file (see https://github.com/niklasstich/Electronize/blob/master/Electronize/Data/ElectronFileSavingService.cs and https://github.com/niklasstich/Electronize/blob/master/ElectronizeTest/SaveTest.cs for the test). But when I was mocking the IWindowManager, I realized that it still returns a BrowserWindow instead of an IBrowserWindow, which means I couldn't mock the class correctly as I wasn't able to mock a BrowserWindow with NSubstitute. Instead, I made a copy of the branch from your pull request and made IWindowManager return an IBrowserWindow instead (see this diff niklasstich@8490c2b).

Is there something I'm missing about this or does this have to be changed in order to mock the WindowManager correctly?

@danatcofo
Copy link
Contributor Author

@niklasstich Yah, I didn't want to go too far down making EVERYTHING mockable as I didn't know dependencies and changing the return to IBrowserWindow in WindowManager|IWindowManager would be considered a breaking change for the repo. Not a big deal but still I was trying to avoid breaking changes.

Your correct that it SHOULD be IBrowserWindow but ... breaking change... I'm not opposed per say of doing this but doing it with some feedback from some heavy contributors or users would be welcome.

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.

3 participants