Skip to content
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

Merged
merged 15 commits into from
Oct 5, 2020

Conversation

joro550
Copy link
Contributor

@joro550 joro550 commented Oct 3, 2020

Pull request description

Adding placeholders that throw exceptions for the following services:

  • NavigationManager
  • HttpClient
  • IStringLocalizer
  • ILoggerFactory

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

  • Pull request is targeting at DEV branch.
  • Pull request is linked to all related issues, if any.
  • I have read the CONTRIBUTING.md document.

Content checklist

  • My code follows the code style of this project and AspNetCore coding guidelines.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have updated the appropriate sub section in the CHANGELOG.md.
  • I have added, updated or removed tests to according to my changes.
    • All tests passed.

egil and others added 8 commits October 1, 2020 16:28
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
@joro550 joro550 changed the title #115 #115 - Adding placeholders for default services Oct 3, 2020
@codecov
Copy link

codecov bot commented Oct 3, 2020

Codecov Report

Merging #223 into dev will increase coverage by 0.00%.
The diff coverage is 81.57%.

Impacted file tree graph

@@           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     
Impacted Files Coverage Δ
...s/Authorization/FakeAuthenticationStateProvider.cs 100.00% <ø> (ø)
...ubles/Authorization/FakeAuthorizationExtensions.cs 100.00% <ø> (ø)
...s/Authorization/FakeAuthorizationPolicyProvider.cs 100.00% <ø> (ø)
...tDoubles/Authorization/FakeAuthorizationService.cs 91.66% <ø> (ø)
...unit.web/TestDoubles/Authorization/FakeIdentity.cs 100.00% <ø> (ø)
...nit.web/TestDoubles/Authorization/FakePrincipal.cs 75.00% <ø> (ø)
...Authorization/MissingFakeAuthorizationException.cs 100.00% <ø> (ø)
...rization/PlaceholderAuthenticationStateProvider.cs 100.00% <ø> (ø)
...s/Authorization/PlaceholderAuthorizationService.cs 100.00% <ø> (ø)
...tDoubles/Authorization/TestAuthorizationContext.cs 94.64% <ø> (ø)
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2611704...59411a2. Read the comment docs.

@egil
Copy link
Member

egil commented Oct 3, 2020

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.

@egil egil self-requested a review October 4, 2020 11:42
Copy link
Member

@egil egil left a 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.

Copy link
Member

@egil egil left a 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.

@joro550
Copy link
Contributor Author

joro550 commented Oct 4, 2020

Hoping that the xml comment will fix the build @egil when you have time I'd appreciate another look 😄

@egil
Copy link
Member

egil commented Oct 4, 2020

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.

@egil
Copy link
Member

egil commented Oct 5, 2020

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.

@egil egil merged commit c8a2fd3 into bUnit-dev:dev Oct 5, 2020
@joro550 joro550 deleted the enhancement/115 branch October 6, 2020 08:03
@egil egil mentioned this pull request Oct 7, 2020
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.

Register stubs of all default services Blazor provides at runtime with useful helper message
2 participants