Umbraco-cms: Netcore: migrate unit and integration tests into new projects

[AndyButland]

Tangentially related to the .Netcore transition, and being worked on in the same netcore/dev branch, are some efforts to clean-up the Umbraco tests. Currently they all sit within the Umbraco.Tests project, and are mix of unit and integration tests.

We'd like to move out the true unit tests into a new project Umbraco.Tests.UnitTests, making it easier for developers to run those tests that can run more quickly, more often.

We're also taking the time to refactor them, by introducing a builder pattern for the construction of model classes used in tests.

A number have already been transitioned, so the necessary patterns have been established and various supporting classes created. But there's quite a number still to do, hence this issue.

The idea is that this issue won't be resolved by a single PR, rather a number of smaller ones, each focused on migrating and refactoring one or a few classes of tests. Given that, it's worth flagging on this issue any tests anyone plans to look at migrating and updating, so effort doesn't get duplicated.

Unit tests can generally be identified as those that either don't inherit from a base class (though there are some that inherit from UmbracoTestBase - normally putting them in the "integration test" category - but that don't really need to, so it's worth checking the test implementation).

The process I've been following when working on the model class tests, using as an example MemberGroupTests, is as follows:

• Pick a unit test class and move it from Umbraco.Tests to Umbraco.Tests.UnitTests.
• Adjust the namespace.
• Remove the inherited base class (if there is one).
• If there are multiple tests all creating the same object to test against, refactor this into a private helper method.
• Add a class level reference to a builder class - e.g. private readonly MemberGroupBuilder _builder = new MemberGroupBuilder();. It's likely this class won't exist yet.
• Change the implementation of model classes construction to use the builder pattern.

E.g. this:

var group = new MemberGroup()
{   
    CreateDate = DateTime.Now,  
    CreatorId = 4,  
    Id = 6, 
    Key = Guid.NewGuid(),   
    UpdateDate = DateTime.Now,  
    Name = "asdf"   
};  
group.AdditionalData.Add("test1", 123); 
group.AdditionalData.Add("test2", "hello");

Becomes:

return _builder
    .WithId(6)
    .WithName("Test Group")
    .WithCreatorId(4)
    .AddAdditionalData()
        .WithKeyValue("test1", 123)
        .WithKeyValue("test2", "hello")
        .Done()
    .Build();

Note that we don't need to explicitly set every property - only the ones specific for our tests - as we can expect the builder class when implemented to set appropriate defaults. If we find the same object being built for multiple tests, we can have further re-use by creating an extension method on the builder class to create a particular test object.

• Then implement the builder class itself - see for reference MemberGroupBuilder in the Umbraco.Tests.Common project - to make the test code compile and pass.
• For common properties, used in different model classes, leverage or consider creating interfaces like IWithIdBuilder.
• As the example shows, builder methods all return an instance of the builder itself, allowing them to be chained. Each one will usually save some provided value into internal state. When Build() is called, the object the builder creates is constructed, using that state.
• Builders can also be nested. In the example above AddAdditionalData returns a new builder which has it's own methods for collecting the necessary data to construct the object, which is done when Done() is called. This returns an instance of the original builder class such that the chain can continue.
• Finally, it's worth creating a test for the builder class itself, in Umbraco.Tests.UnitTests, to verify that the built objects are constructed correctly.

---

Similarly we're looking to migrate the integration tests - those that inherit from a base class that spins up a database to test against - into their own project: Umbraco.Tests.Integration.

There's a comment below linking to an example of this - but generally the approach is to:

• Move the test into the appropriate project.
• Amend the base class and add the necessary usings.
• Remove calls to ServiceContext and replace with uses of GetRequiredService.
• Refactor to use builders where appropriate to instantiate test model classes.
• Check the tests still pass.

[umbrabot]

Hi @AndyButland,

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 :-)

[emmagarland]

Working on UmbracoSettings unit tests

[electricsheep]

Starting work on moving tests in the Misc folder. @AndyButland - if tests are touching the file system, should we keep them in the unit test project or be moving them to the integration test project?

[AndyButland]

Hmm... good question. I guess you could argue that strictly they aren't unit tests with a file system dependency, but I think the more important split here is between "fast tests" and "slow tests". So I'd suggest they should go in the unit tests if all they are doing is for example reading a test data file from disk. @bjarnef may want to comment though.

[bjarnef]

@AndyButland I think you wanted to mention @bergmania ? 😁

[AndyButland]

Oops, yes I did! Thanks for correcting.

[bergmania]

@electricsheep good question. I agree with @AndyButland, the most important think is actually that the tests are fast.

No matter whether these tests is located in UnitTests or IntegrationTests, I think they should be seperated from real unittests or db-integration tests, respectively.

If you start by migrating them into UnitTests, it would be perfect.

[electricsheep]

Picking up some more low-hanging fruit from test folders:

• Clr
• Collections
• Composing

@AndyButland @bergmania should I be using the builder pattern in cases where a test class is building the same object and the object includes mocked dependencies? e.g.

var foo = new Foo(Mock.Of<IBar>());

[bergmania]

@electricsheep, not necessary, can you give a more concrete example, then I can take a look and give my thoughts

[electricsheep]

@electricsheep, not necessary, can you give a more concrete example, then I can take a look and give my thoughts

Sure, the class I'm looking at is CompositionTests where I have a helper method like this:

        private static Composition BuildComposition(RuntimeLevel level)
        {
            var mockRuntimeState = new Mock<IRuntimeState>();
            mockRuntimeState.Setup(x => x.Level).Returns(level);
            return new Composition(MockRegister, MockTypeLoader, Mock.Of<IProfilingLogger>(), mockRuntimeState.Object, Mock.Of<Configs>(), MockIOHelper, AppCaches.NoCache);
        }

Another thing I wanted to check about in that class is whether we should keep classes created specifically for the test class in the class e.g from the same class as above:

        public class Component5a : TestComponentBase
        {
        }

[bergmania]

@electricsheep
A CompositionBuilder could make sense. So we dont have a think about all these mocks/dependencies everytime.

This example could be something like

return new CompositionBuilder()
  .AddRuntimeState()
    .WithLevel(RuntimeLevel.Run)
  .Done()
  .Build();

Regarding the classes specific for the test class, then I think it is best if they are still located in the same file.

[JimBobSquarePants]

c# return _builder .WithId(6) .WithName("Test Group") .WithCreatorId(4) .AddAdditionalData() .WithKeyValue("test1", 123) .WithKeyValue("test2", "hello") .Done() .Build();

Isn't this a lot of boilerplate to introduce (which then requires additional tests for each builder)?

[AndyButland]

I guess you could see it as extra boiler-plate, but the thinking is that if there's one builder per model class, but potentially many tests that use it, it'll pay for itself - in consistency, and less setup required in creating the class instances. Perhaps not so clear from when I created this issue, but the idea also is that the builder can create sensible defaults, so whereas without them you'd have to specify certain property values explicitly, with the builder you can only specify what you want that's different from the defaults.

@bergmania has described the idea behind introducing this it a bit more in this post (see under "Maintainability"). Patterns like this are to an extent "in the eye of the beholder" I'm sure, but personally I quite like the approach.

[JimBobSquarePants]

Personally I would use partial classes for any tests and a single shared factory method. If you require different parameters then they can be passed to that factory method as part of a theory data collection.

[bergmania]

Personally I prefer builders. I agree with what @AndyButland says.

There exists some frameworks out there that can do the builder for you, like AutoFixture. Personally I prefer to create the builders my self, to have full control and remove that little piece of "magic" they introduce.

[bergmania]

A simple example of how to migrate an integration test of a service:
https://github.com/umbraco/Umbraco-CMS/commit/d74e18a15d833ebe0b295e6d5bf2c899bb4a2605

[AndyButland]

If anyone is planning to work on this as part of the upcoming Umbraco Hackathon, there's a Google Doc here to track what you and others are working on. The idea being that people don't accidentally end up overlapping and working on the same classes.