UmbracoApiController Testable #6764
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n the UmbracoApiControllerBase constructor. * Added Obsolete marker to old UmbracoApiController constructor injecting Current.Mapper for backward compability, use new full construcor injection instead.
…ted UmbracoMapper * Added test for mocking UmbracoApiController dependencies with ServiceLocator resolved UmbracoMapper for backward compability.
Adolfi
changed the title
Shazwazza
suggested changes
Oct 21, 2019
| ) | ||
| { } | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="UmbracoApiControllerBase"/> class with all its dependencies. | ||
| /// </summary> | ||
| protected UmbracoApiControllerBase(IGlobalSettings globalSettings, IUmbracoContextAccessor umbracoContextAccessor, ISqlContext sqlContext, ServiceContext services, AppCaches appCaches, IProfilingLogger logger, IRuntimeState runtimeState, UmbracoHelper umbracoHelper) | ||
| protected UmbracoApiControllerBase(IGlobalSettings globalSettings, IUmbracoContextAccessor umbracoContextAccessor, ISqlContext sqlContext, ServiceContext services, AppCaches appCaches, IProfilingLogger logger, IRuntimeState runtimeState, UmbracoHelper umbracoHelper, UmbracoMapper umbracoMapper) |
There was a problem hiding this comment.
Unfortunately you're going to have to obsolete this ctor as well and add another overload, just like you did with the UmbracoApiController
| } | ||
|
|
||
| [Test] | ||
| public void Can_Mock_UmbracoApiController_Dependencies_With_ServiceLocator_UmbracoMapper() |
There was a problem hiding this comment.
We prob don't need this test
* Added back the old construcor in UmbracoApiControllerBase and added a obsolete attribute.
|
Hi @Shazwazza I updated this PR to your feedback. |
|
Hey @Adolfi Thanks for the work here, and for the update too! We'll take a look and shout if we have any further suggestions or questions. Emma |
|
@Adolfi great stuff! i'll merge in , thank you 🎉 |
|
Thanks @Shazwazza! 👍🏻 Any chance you could squeeze in umbraco/UmbracoDocs#1987 also? I don’t mean to stress it but since the current documentation promotes bad practice it would be nice to get this one out as soon as possible. Cheers!! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
From discussion on #6578 (comment) it turned out that the UmbracoApiControllerBase used the ServiceLocator to resolve the UmbracoMapper (Current.Mapper) https://github.com/umbraco/Umbraco-CMS/blob/v8/dev/src/Umbraco.Web/WebApi/UmbracoApiControllerBase.cs#L62, which made it impossible to mock/fake (unless you mock the Current.Mapper, which you shouldn't).
This PR injects the UmbracoMapper in the constructor instead and adds a obsolete attribute on the old constructor, which is kept for backward compatibility.
One tests has been added to verify that it is possible to mock the new constructor but also one that verifies that the old constructor can still resolve the Current.Mapper dependency, again for backward compatibility.
@Shazwazza was this what you had in mind?
Cheers.