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

Add unit test project to solution #299

Closed
mrlacey opened this issue May 11, 2017 · 28 comments
Closed

Add unit test project to solution #299

mrlacey opened this issue May 11, 2017 · 28 comments
Assignees
Labels
Can Close Out Soon Work relating to this issue has been completed. feature The issue is a request for a new feature in the generated projects. A feature is different from a pa
Milestone

Comments

@mrlacey
Copy link
Collaborator

mrlacey commented May 11, 2017

Add a feature to add a unit test project to the solution. This should include a reference to the app project.

Need to consider what, if any, test(s) to include by default.

@mrlacey mrlacey added the feature The issue is a request for a new feature in the generated projects. A feature is different from a pa label May 11, 2017
@jamesmcroft
Copy link
Member

Similar to my comments in #300 - would this be optional as it currently is when creating a new blank project?

@crutkas crutkas added this to the Backlog milestone May 13, 2017
@crutkas
Copy link
Member

crutkas commented May 14, 2017

why not just allow someone to opt in as a system level and make the whole project straight up testable. With that said, I can see how to do UI testing, but to do stub out a proper unit test without any business logic? feels odd as not sure how much would be testable

@mrlacey
Copy link
Collaborator Author

mrlacey commented May 16, 2017

My thinking was that by making this a "feature" it becomes optional.
Yes, there's not a lot we can test but by making it easy to add unit testing to the solution we can hopefully help with its adoption.

Even if we only added a single test which included Assert.Inconclusive("TODO: Write some tests"); I would consider this better than doing nothing.

@magol
Copy link

magol commented May 17, 2017

Perhaps add some commented examples of some types of tests that you can perform just to show how to best write tests based on selected conditions.

The conditions for writing tests differ depending on the chosen framework (Code behind, MVVM Basic, MVVM light, PRISM), how Dependency injection is performed, chosen unit test and mock framework, etc.

@mrlacey
Copy link
Collaborator Author

mrlacey commented Mar 30, 2018

Once we get to FCU as a minimum SDK and split out potentially X-Plat functionality this could be 3 options:

  • Add a UWP unit test project that references the main app for testing the functionality it contains
  • A NetCore 2.0 project using MSTest that references the X-Plat library for testing X-Plat functionality
  • A NetCore 2.0 project using XUnit that references the X-Plat library for testing X-Plat functionality

@mrlacey mrlacey modified the milestones: Backlog, 2.1 Mar 30, 2018
@mrlacey
Copy link
Collaborator Author

mrlacey commented Apr 11, 2018

moving back to backlog as timeline for major restructuring is now undetermined

@crutkas
Copy link
Member

crutkas commented Nov 5, 2018

@mrlacey, will this be possible to be done for 3.0?

@mrlacey
Copy link
Collaborator Author

mrlacey commented Nov 5, 2018

Just like #300 this is blocked by the NuGet issue that is preventing us generating multi-project solutions. Not necessary for 3.0 so moving to 3.1

@mrlacey mrlacey modified the milestones: 3.0, 3.1 Nov 5, 2018
@mrlacey mrlacey self-assigned this Jan 4, 2019
@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 4, 2019

Thinking about what to name the projects added to the solution and need them to be unique (so we can add them all to a solution--even if only in testing) but meaningful.

I'm thinking:

  • {App}.Tests.Unit.MSTest (would reference {App}.Core)
  • {App}.Tests.Unit.NUnit (would reference {App}.Core)
  • {App}.Tests.Unit.xUnit (would reference {App}.Core)
  • {App}.Tests (Would reference {App} and be a "Universal Windows Unit Test App")

Using the common prefix of {App}.Tests.Unit followed by the framework name for tests that reference the "core" .Net Standard project.
With the last suggestion, I didn't want to use "Unit" in the name as the fact it relies on UI infrastructure makes it hard to consider it a place for isolated unit tests.

I'd be happy to drop the ".Unit" element from the compound names above if people don't think it adds value.
Should the last suggestion above include something to indicate that it relates to the main app project?

Note. The generated UI Test project is called {App}.Tests.WinAppDriver and so these new test projects should fit the pattern this has established.

speak up if you have objections or alternate suggestions.

@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 8, 2019

My thought for "Unit" tests is to just include a simple test that checks something is returned by the sample data service:

    var actual = SampleDataService.GetSampleModelDataAsync();

    Assert.IsNotNull(actual);
    Assert.AreNotEqual(0, actual.Count);

This would mean adding a page that uses the SampleDataService or making SampleDataService.AllOrders public and including it as a dependency of the test.


For the UWP test project, include a test like this so it's enough to show access to the contents of a ViewModel. For the CodeBehind version it could test the property on the SettingsPage.
This would require a dependency on the settings page though. As one of the most commonly added pages this shouldn't be too much of an issue. (hopefully)

    var sut = new ViewModels.SettingsViewModel();

    var actualDescription = sut.VersionDescription;

     Assert.IsFalse(string.IsNullOrWhiteSpace(actualDescription));

Both these suggestions require adding a dependency to other pages. While including unneeded pages isn't great, if we want to include example tests that actually test something useful, I don't see another option as there isn't anything appropriate to test in the base generated project.
Adding something to the generated code that's only used by the test seems pointless. Adding a page just to show how to test it is only a slight improvement but I can't see a better alternative.

Thoughts? Suggestions?

@sibille
Copy link
Collaborator

sibille commented Jan 10, 2019

@mrlacey for the naming of test projects I'd drop the Unit part and add the full name of the tested project, like this:

• {App}.Core.Tests.MSTest (would reference {App}.Core)
• {App}.Core.Tests.NUnit (would reference {App}.Core)
• {App}.Core.Tests.xUnit (would reference {App}.Core)
• {App}.Tests (Would reference {App} and be a "Universal Windows Unit Test App")

Regarding the dependencies:
Instead of adding one of the real pages, I'd add only the SampleDataService for core tests and a special SampleTestPage for the Uwp tests, as this is easier to remove. Especially the settings page adds a lot of code to remove if the dev doesn't want it (ThemeService, resources, changes in ShellPage.xaml).

@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 10, 2019

Latest thinking:

  • By default, include a simple, example test that is unrelated to the generated code. This is the same as what you'd get if you did: File > New > Test Project
  • If other options are selected as part of the initial generation and that we can include a simple, but potentially useful test for (see examples in earlier comments) we include the additional tests as well.
  • This removes the need to create unnecessary dependencies.
  • This also removes any issue with knowing what was preinstalled when doing a right-click>add
  • A potential downside is that the tests included in a generated project are inconsistent, depending on what else is selected. Being able to provide more tests by default would be nicer, but WTS also has the goal of including nothing unnecessary in the generated code and so adding things so that they can be tested seems very wrong. I also don't want to add tests that have no real value just to increase numbers.

I also like the idea of being able to have the generated code link to some documentation that would help anyone getting started with unit testing but it's such a big topic, I wouldn't want to try and write something myself. I can't even find any suitable docs to link to. :'(

@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 10, 2019

Regarding the dependencies:
Instead of adding one of the real pages, I'd add only the SampleDataService for core tests and a special SampleTestPage for the Uwp tests, as this is easier to remove. Especially the settings page adds a lot of code to remove if the dev doesn't want it (ThemeService, resources, changes in ShellPage.xaml).

SampleDataService only exposes anything public when there are other items that use it. I want to avoid changing this or deal with exposing internals as that's often considered an anti-pattern.

Adding a separate page just to demonstrate how to test something also feels wrong. Whether it's a new or existing one I just don't think we should do it. We've decided against including things just for the sake of demonstration in the past. (The potential exception being deep linking but what we added made the feature complete. It will still be possible to use and run the tests in the test project without including dependencies on parts of the app.) If we had a separate selectable item which was "demo test content" then maybe I'd be ok with that but including such a thing would set a dangerous precedent for other templates and requests for support and documentation.

Will review all pages and features for possible test integration.

@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 10, 2019

Think we should also have versions of the app tests for each framework too.
This means adding a total of six new options/projects.

{App}.Core.Tests.MSTest (would reference {App}.Core)
{App}.Core.Tests.NUnit (would reference {App}.Core)
{App}.Core.Tests.xUnit (would reference {App}.Core)
{App}.Tests.MSTest (would reference {App})
{App}.Tests.NUnit (would reference {App})
{App}.Tests.xUnit (would reference {App})

in addition to

This means there's the potential to add seven additional projects to a generated solution. We'd do this in tests, but the most that makes reasonable sense for someone to include would be three (one for the app, one for the core lib, and one for the UI-with WAD.) I'll be keen to see the telementry on what people do actually add though.

@mrlacey mrlacey changed the title Feature: Add unit test project to solution Add unit test project to solution Jan 10, 2019
@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 11, 2019

I'd thought about making test frameworks mutually exclusive but I don't see any real benefit to it.
It would be more work for us and it would require more complexity in testing.
I have, however, worked on code bases in the past where we had multiple test projects using different frameworks because we needed different features of different frameworks. It's a niche scenario but I don't see any reason to exclude it.
Similarly, I doubt anyone would really include all the notification features in a single project but we don't stop them.

There are other features that have been discussed separately where a genuine mutual exclusion of options would be necessary but as we don't need that yet, lets focus on providing end user (developer) functionality with the infrastructure we already have.

With regard composition, I haven't looked at it in detail but had a few ideas.
In order of preference:

  1. Extend filtering to allow the specification of multiple identities and only include them if all identities are in the userselection.
  2. Work with (or extend) the recent work on optional context lines in merge postactions.
  3. Add the ability to use the existence of a file (including path name replacements) in a compostion filter.
  4. Work with a modified version of the search&replace post action.

mrlacey added a commit to mrlacey/WindowsTemplateStudio that referenced this issue Jan 15, 2019
@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 15, 2019

Have started experimenting with adding tests for the core project.

image

image

mrlacey added a commit to mrlacey/WindowsTemplateStudio that referenced this issue Jan 17, 2019
@mrlacey mrlacey added the in-progress The issue is currently being actively worked on. label Jan 17, 2019
@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 18, 2019

image

Have modified the default .Net Core test project icons to aid differentiation by icon.

mrlacey added a commit to mrlacey/WindowsTemplateStudio that referenced this issue Jan 18, 2019
@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 19, 2019

VB versions of Core test projects
image

@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 21, 2019

Adding example tests based on other selected items

What

If adding a test feature and select a page that adds content we can add an example test for, we should add such a test.

Example.
If select a Chart page and the Core MSTest feature, add a test like this (in addition to the default test)
image

How

By extending GenComposer to do context filtering based on the identity of selected pages.

Doing so means the ability to have a composition filter like this:

"wts.compositionFilter": "$framework == MVVMBasic|MVVMLight|CodeBehind|CaliburnMicro & identity == wts.Feat.UnitTests.Core.MSTest & $page == wts.Page.Chart|wts.Page.Chart.CodeBehind",

This includes the composition template if using one of the defined frameworks, have included an item with the identity of "wts.Feat.UnitTests.Core.MSTest" and have also added a page with the Identity of "wts.Page.Chart" or "wts.Page.Chart.CodeBehind".

Just including one of either the chart page or the test library feature and the generator does not attempt to add the extra test.

The order of selected items doesn't matter. You can add the page, then the feature, or the other way round.
You can even add multiple copies of the page, and it still works fine.

If this filter was used with a template identity that allows multiple instances there is the possibility that this could break things as it might try and add duplicate items. It shouldn't have an issue for the functionality I'm adding here. I don't think it's something we should prevent as it might be desirable for other scenarios. Our template tests would catch anything that caused a breaking build, but this is potentially powerful and will need a note in the documentation.

This would work with Right-click>Add but with a slight difference
When adding to a previously generated app we don't know what other pages (or features) were originally selected and so we could not add the composition functionality. In this instance, if adding to an app that had a chart added when originally generated we'd add the test framework but without the additional test relating to chart sample data.
**This would mean we have a feature that potentially behaves differently on the original generation and when adding to a previously created app. I don't see this as being a problem; I'm just calling it out. This is also the first time we have generated code that is dependent upon multiple pages/features without requiring a dependency between them.

Attempting to conditionally add the test when doing a right-click addition would be complicated and error-prone.
The above-defined solution involves only a very small change to the generator code base in an area that is already well tested. After exploring many options, this seems to be the best way to go and also involves the smallest code changes.

I only need $pages support now, but I wonder if I should also all support for filtering by $feature now as it may be needed in the future too.

@sibille thoughts on this approach?

@sibille
Copy link
Collaborator

sibille commented Jan 22, 2019

Looks correct to me!
I'd add in $pages and $features support.
Also agree on right click should only add basic tests.

I was initially thinking about approaching this differently:
My thought was that there could be pages/features that add context variables others can act upon,
the test template would add a $testframework variable and the Chart page would have a composition template that adds the tests for this framework.

Composition filter would look like this:
"wts.compositionFilter": "$framework == MVVMBasic|MVVMLight|CodeBehind|CaliburnMicro & identity == wts.Page.Chart|wts.Page.Chart.CodeBehind & $testframework == MSTest"

I think both approaches are valid, we should look into advantages for each one.
My concern is that we'll have a lot of composition templates for different test frameworks. Is there a way to share code between test composition templates?

@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 22, 2019

Ok. Two issues to resolve

  1. Filter by included page/feature or add a specific $testframework filter?
  2. Managing shared code between a large number of similar composition templates.

Filtering

Adding a specialized $testframework filter to the generation context adds specialized filtering and extra complexity to generation. It risks duplicating the same functionality as a $feature or $page filter but without the option to extend it to other features. We'd also need to add a new wts.type value for "test." If the plan is to treat test projects like any other feature, this feels like unnecessary complexity and a limitation for future extensibility with other features.
If test projects are going to be broken out into a separate step (#2841) then the argument for filtering as separate wts.type is stronger but this isn't something I expect to happen for several months.
Adding a $testframework filter now is more work now, and it's potentially unnecessary work if tests aren't broken into a separate step at a later point. Filtering by $feature/$page now is less work and would still allow for breaking into a separate step at a later point, if wanted.

Managing shared code

To add all the sample data tests via the different options is going to require 36 new composition templates. (I think--I've so far only made 7 of them to explore options.)
All these templates only add a single file. Each file contains six lines (not including comments & curly braces) and differs in 5 of them. I see little value in trying to reduce the reuse of a couple of lines, even if they are repeated in dozens of files. It would add more complexity for very little obvious benefit.

Alternatives to reduce the number of composition files being added here:

  • Add a test of something that applies to more scenarios. There isn't anything, and we don't want to add things just to be tested.
  • Change the generated code (in other templates), so we can have tests that work in more scenarios. This could break right-click>add for previously generated apps even if not using tests.

These options were previously ruled out as being undesirable.

As these (36) composition templates are all very small, the chances of needing to change anything in them (one of the main reasons for reducing duplication) is also very small.
For these reasons, I don't see any need to do anything more to investigate new ways to share code in templates at this time.

@mrlacey mrlacey closed this as completed Jan 22, 2019
@mrlacey mrlacey reopened this Jan 22, 2019
mrlacey added a commit to mrlacey/WindowsTemplateStudio that referenced this issue Jan 25, 2019
Also updates VB style checks to run on all projects in the solution & fix issues they found
Also added build tests to run all the tests after building with all pages and features
For microsoft#299
@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 28, 2019

Adding tests for code in the app projects is currently blocked by microsoft/microsoft-ui-xaml#230

@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 29, 2019

The ability to use NUnit to create tests against the app cannot be added at this time as NUnit does not support it, yet.
There is a long-running open issue for NUnit adding support at nunit/nunit3-vs-adapter#322

This and #300 means we'll only be adding 6 new features relating to Testing
image

@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 30, 2019

I'm reviewing what's appropriate to add to the App tests project based on other selected items.

There is very little that's easy or appropriate to test in the View Models that are produced by default.
I'd really like to include something for each though. Just creating an empty project and saying "this is where you put tests" doesn't seem adequate.
My instinct is to include a test for every ViewModel (or page for CodeBehind) but only include the minimum possible to justify calling it a valid test.

For example, adding a Camera Page/ViewModel would give you this:

        // TODO WTS: Add tests for functionality you add to CameraViewModel.
        [TestMethod]
        public void TestCameraViewModel()
        {
            // This test is trivial. Add your own tests for the logic you add to the ViewModel.
            var vm = new CameraViewModel();
            Assert.IsNotNull(vm);
        }

It adds a test to the test project.
It points to areas that tests should be created for
It acknowledges that what's provided is simple but the value of tests will be for the code added by the developer.

@sibille @crutkas thoughts?

@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 30, 2019

Because some Prism ViewModels require the injection of dependencies, I'm using Moq to create mock instances of these dependencies.
On the plus side it feels a little bit closer to providing something more useful in the default test.

mrlacey added a commit to mrlacey/WindowsTemplateStudio that referenced this issue Jan 30, 2019
mrlacey added a commit to mrlacey/WindowsTemplateStudio that referenced this issue Jan 30, 2019
@mrlacey
Copy link
Collaborator Author

mrlacey commented Jan 31, 2019

Waiting for #2527 and then will submit PR

@mrlacey mrlacey added Can Close Out Soon Work relating to this issue has been completed. and removed in-progress The issue is currently being actively worked on. labels Feb 15, 2019
@mrlacey
Copy link
Collaborator Author

mrlacey commented Mar 4, 2019

verified in
Templates version: 0.20.19063.1
Wizard version: 0.20.19061.1

@mrlacey mrlacey closed this as completed Mar 4, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Jun 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Can Close Out Soon Work relating to this issue has been completed. feature The issue is a request for a new feature in the generated projects. A feature is different from a pa
Projects
None yet
Development

No branches or pull requests

5 participants