-
-
Notifications
You must be signed in to change notification settings - Fork 109
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
#115 - Adding placeholders for default services #223
Conversation
Adding placeholder navigation manager that throws a helpful exception to prompt users to go look at the docs
Implementing placeholder httpclient that throws exceptions that prompts users to read the documentation
Adding placeholder log factory that throws exceptions to prompt users to create a mock
Added placeholder string localizer that throws exceptions that prompt the user to look at documentation
Codecov Report
@@ Coverage Diff @@
## dev #223 +/- ##
=======================================
Coverage 79.24% 79.25%
=======================================
Files 103 109 +6
Lines 3011 3046 +35
Branches 394 396 +2
=======================================
+ Hits 2386 2414 +28
- Misses 497 503 +6
- Partials 128 129 +1
Continue to review full report at Codecov.
|
Hey Mark Thanks for this. A few quick notes, will provide more feedback tomorrow. Make the placeholder classes internal. Nobody needs to new them up or use them outside of bUnit. About the docs, yeah, I ideally we add a quick guide on how to create mocks for each of these in the docs, using e.g. Moq or NSubstitute. @DarthPedro is actually planning to create a guide for navigation manager. |
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.
Hi @joro550
Kudos for matching the coding style almost exactly, and I only have a few things I would like you to improve before we can merge.
As for the logger placeholder, lets just remove it. It doesn't really make sense I think. There is already one in the library, and too much is in bUnit depends on this, so lets leave this out.
Thanks, Egil
Ps. I am pushing a commit to the PR branch with documentation for the HttpClient mock.
src/bunit.web/TestDoubles/HttpClient/PlaceholderHttpMessageHandler.cs
Outdated
Show resolved
Hide resolved
src/bunit.web/TestDoubles/HttpClient/MissingMockHttpClientException.cs
Outdated
Show resolved
Hide resolved
src/bunit.web/TestDoubles/HttpClient/MissingMockHttpClientException.cs
Outdated
Show resolved
Hide resolved
src/bunit.web/TestDoubles/NavigationManagement/PlaceholderNavigationManager.cs
Outdated
Show resolved
Hide resolved
src/bunit.web/TestDoubles/NavigationManagement/MissingMockNavigationManagerException.cs
Outdated
Show resolved
Hide resolved
src/bunit.web/TestDoubles/HttpClient/PlaceholderHttpMessageHandler.cs
Outdated
Show resolved
Hide resolved
src/bunit.web/TestDoubles/JSInterop/MockJSRuntimeInvokeHandler.cs
Outdated
Show resolved
Hide resolved
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.
Looks good. A few more things. Respond to this comment when you want me to look at this again.
Hoping that the xml comment will fix the build @egil when you have time I'd appreciate another look 😄 |
Will do. The build script builds in release mode, so you can find any build errors locally by switch to it. |
…to avoid users having to import many namespaces, cleaned code a little
This looks good now @joro550, thanks for all your work. I tweaked a few things slightly while reviewing, and moved all testdoubles into the same namespace, such that users do not have to import many different. |
Pull request description
Adding placeholders that throw exceptions for the following services:
Re: ILoggerFactory
I have created a placeholder logger factory but if I add it into the service collection then many of the tests fail - this is because there are requests outside of the components that make requests for the log factory, so for now I have commented that association out and will wait further instruction
Re: Documentation & changelog
There is a whole heap of documentation to do off the back of this I suspect so I'll try my best at adding some good information, thought it might be good to have the code reviewed
Thanks!
PR meta checklist
DEV
branch.Content checklist