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

Assignment4 #88

Closed
wants to merge 2 commits into from
Closed

Conversation

@dsergio
Copy link

dsergio commented Feb 4, 2020

No description provided.

dsergio added 2 commits Feb 4, 2020
@GamerBah

This comment has been minimized.

Copy link

GamerBah commented Feb 6, 2020

Will review!

@GamerBah

This comment has been minimized.

Copy link

GamerBah commented Feb 6, 2020

Assignment 4

  • WebAPI Analyzers enabled ✔
  • DI properly configured
    • Services properly registered with correct scopes ✔
    • DB context properly registered ✔
    • AutoMapper properly registered ✔
  • NSwag properly setup ✔
  • API controllers properly expose CRUD operations ✔
  • Controllers are all unit tested ✔

Extra Credit

  • Unit tests in EntityServiceTests fixed approprietly ✔
  • Mocking framework used to create the test doubles

I wasn't able to find where you used Moq, even though it's included in your packages and included in 2 of the controller test classes. Maybe I'm just looking over it, somehow?

{
[Route("api/[controller]")]
[ApiController]
public class EntityController<TEntity> : ControllerBase where TEntity : class

This comment has been minimized.

Copy link
@GamerBah

GamerBah Feb 6, 2020

Good job writing the controller generically for Entity, and extending that in the other controller classes.

namespace SecretSanta.Api.Tests.Controllers
{
[TestClass]
public class GiftControllerTests

This comment has been minimized.

Copy link
@GamerBah

GamerBah Feb 6, 2020

You can write generic test classes for all the controllers too, just like writing a generic controller class. This is the site I found very useful in explaining how to do it. Super handy for not having to write the same tests for different sub-classes.

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

Agreed, in general if there is a polymorphic relationship between the classes you are testing the same relationship can/should be applied to the tests.

Copy link
Collaborator

Keboo left a comment

(dsergio)

Assignment4

Project Analysis Summary
Nullable Enabled:
Unexpected Disabled Warnings:
Appropriately handle all warnings:
ProjectName Nullable Solutions Analyzers Unexpected Disabled Warnings Waning Level
SecretSanta.Api Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.Analyzers, Microsoft.CodeAnalysis.CSharp.Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Business Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Data Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Web Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer 4
SecretSanta.Api.Tests Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.Analyzers, Microsoft.CodeAnalysis.CSharp.Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Business.Tests Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Data.Tests Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer, Microsoft.EntityFrameworkCore.Analyzers 4
SecretSanta.Web.Tests Enable SecretSanta.sln IntelliTect Analyzers, Microsoft CodeQuality Analyzers, Microsoft .NET Core Analyzers, Microsoft .NET Framework Analyzers, Microsoft.CodeAnalysis.VersionCheckAnalyzer 4

Main Assignment

  • WebAPI Analyzers enabled ✔
  • DI properly configured ✔
  • Services properly registered with correct scopes ✔
  • DB context properly registered ✔
  • AutoMapper properly registered ✔
  • NSwag properly setup ✔
  • API controllers properly expose CRUD operations ✔
  • Controllers are all unit tested ✔

Extra Credit

  • Unit tests in EntityServiceTests fixed appropriately ✔
  • Mocking framework used to create the test doubles

Peer Review

  • On time ✔
  • Accurate ✔
  • Appropriate comments given ✔
return Task.FromResult(Items[id]);
}

public Task<User[]> InsertAsync(params User[] entity)

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

Parameter entity of method InsertAsync is never used. Remove the parameter or use it in the method body.

return Task.FromResult(Items[id]);
}

public Task<Group[]> InsertAsync(params Group[] entity)

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

Parameter entity of method InsertAsync is never used. Remove the parameter or use it in the method body.

@@ -2,6 +2,7 @@
<PropertyGroup>
<TargetFramework>netcoreapp3.1</TargetFramework>
<Nullable>enable</Nullable>
<IncludeOpenAPIAnalyzers>true</IncludeOpenAPIAnalyzers>

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

This certainly woks, but outside of web projects it does not add any value

CreateMap<User, User>().ForMember(property => property.Id, option => option.Ignore());
CreateMap<Gift, Gift>()
.ForMember(property => property.Id, option => option.Ignore())
.ForMember(property => property.CreatedBy, option => option.Ignore());

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

Though this solution certainly works to make the unit tests happy, it does expose some problems. For example it still allows someone outside of the data layer to directly modify the finger printing properties (which really should be completely controlled in the data layer). You can see a solution in the Assignment4Complete branch.

namespace SecretSanta.Api.Tests.Controllers
{
[TestClass]
public class GiftControllerTests

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

Agreed, in general if there is a polymorphic relationship between the classes you are testing the same relationship can/should be applied to the tests.

@@ -14,6 +15,7 @@
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
<PackageReference Include="NSwag.AspNetCore" Version="13.2.2" />

This comment has been minimized.

Copy link
@Keboo

Keboo Feb 11, 2020

Collaborator

This should not need to get applied outside of the web projects.

@Keboo Keboo closed this Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.