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

Unit testing documentation is wrong #1879

Closed
DontPanic345 opened this issue Sep 26, 2019 · 10 comments
Closed

Unit testing documentation is wrong #1879

DontPanic345 opened this issue Sep 26, 2019 · 10 comments

Comments

@DontPanic345
Copy link

This is the page with issues: https://github.com/umbraco/UmbracoDocs/edit/master/Implementation/Unit-Testing/index.md

This page is totally unhelpful

current doesn't exist at all.

@sofietoft
Copy link
Contributor

Oh no @DontPanic345 ! 😱
Sorry to hear this isn't helpful to you.

Which version of Umbraco are you running it on?
And do you have any more details that could help us identify where this article needs updating?

@DontPanic345
Copy link
Author

Hi @sofietoft, thanks for the reply

I'm using 7.15.1, and was having a lot of trouble getting my tests to run inside Visual Studio, used this page as a place to make my gripe.

I think I have it figured out now by following https://github.com/lars-erik/umbraco-unit-testing-samples and coping the adapter class in there.

This page could use a little something to say that it is for 8.

But doesn't look like valid C# to me the Current.Factory isn't coming from anywhere, I think the doc could benefit from showing the entire file.

@sofietoft
Copy link
Contributor

Aah, good point @DontPanic345 !
We should have a way to show that an article is for Umbraco 8.

Good to hear you managed to figure out how to run it in Umbraco 7!
It would be awesome if you would make an article on it for that version 😁 Just in case someone else might be looking to do the same.

@Shazwazza
Copy link
Contributor

Ideally we should update these unit testing docs to show best practices examples. For the most part we don't want to be initializing singletons. In some cases this may be needed but that would generally only be for integration test. Unit testing is done on the premise of constructor injection which these examples do not showcase.

I've written up an explanation of this here umbraco/Umbraco-CMS#6578 (comment) and followed up more here umbraco/Umbraco-CMS#6578 (comment)

We have some great documentation about dependency injection hidden away in this article here https://our.umbraco.com/documentation/Implementation/Services/#custom-services-and-helpers-1 ... most of this information should be copied and/or migrated to the dependency injection docs here: https://our.umbraco.com/Documentation/Reference/Using-Ioc/ and also to the unit testing docs.

For example, testing controllers should be using full constructor injection. The reason why this article says you will get an exception is purely because these things are being tested with their default constructors and not their full constructors which they should be. There's an example of ctor injection being done in the docs way down here https://our.umbraco.com/Documentation/Implementation/Unit-Testing/#dictionaries with the MyDictionaryDependentController but that is still not using full ctor injection for proper testing. To do that, the full underlying contructor with all depednencies needs to be overrridden. If you look at this link and scroll down a bit until you see the "Tip", it has a GIF showing how you can auto-generate the full overridden constructor of any class: https://our.umbraco.com/documentation/Implementation/Services/#using-the-custom-siteservice-inside-a-controller

@umbrabot
Copy link

umbrabot commented Oct 9, 2019

Hi @DontPanic345,

We're writing to let you know that we've added the Up For Grabs label to your issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post

Thanks muchly, from your friendly PR team bot :-)

@Adolfi
Copy link
Contributor

Adolfi commented Oct 10, 2019

I'm working on a PR update for this page where all tests are using full constructor injection,
which will solve a few issues:

  • Avoid bad practice of mocking static service locator (Current.Factory).
  • Parallel execution of tests will work.
  • I've also added a note that this page is only valid for version 8, to avoid misunderstanding that these tests would work for version 7 (like @DontPanic345 had problems with.)

Should have this PR ready before the weekend or beginning of last week.
Cheers!

@sofietoft
Copy link
Contributor

That's fantastic @Adolfi 😄 🎉

Looking forward to review your work!!

@Adolfi
Copy link
Contributor

Adolfi commented Oct 15, 2019

#1987

@Adolfi
Copy link
Contributor

Adolfi commented Nov 27, 2019

This issue could probably be closed now @sofietoft.

@sofietoft
Copy link
Contributor

Ah, I think you're right! 👌 Good catch @Adolfi 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants