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

Beware when mocking in unit tests! #30456

Open
ArticadAlistair opened this issue Aug 2, 2022 · 6 comments · May be fixed by #43694
Open

Beware when mocking in unit tests! #30456

ArticadAlistair opened this issue Aug 2, 2022 · 6 comments · May be fixed by #43694
Assignees
Labels
doc-enhancement Improve the current content [org][type][category] dotnet-csharp/svc in-pr This issue will be closed (fixed) by an active pull request. Pri2 📌 seQUESTered Identifies that an issue has been imported into Quest. whats-new/subsvc

Comments

@ArticadAlistair
Copy link

ArticadAlistair commented Aug 2, 2022

When one's logging class implements an interface, and that interface is used to make a mock for unit testing, the addition of the "public void LogMessage(LogLevel level, LogInterpolatedStringHandler builder)" method prevents a mock from being fully implemented. This is because the LogInterpolatedStringHandler is a ref struct, and mocking frameworks will not allow ref structs to be used as arguments to mocked methods. This could be a serious problem for some people. I solved it in my case by replacing all mocked instances with the real thing, which was about 2 hours' work, and left my unit tests dependent on another class, which I didn't appreciate.


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.


Associated WorkItem - 345701

@dotnet-bot dotnet-bot added the ⌚ Not Triaged Not triaged label Aug 2, 2022
@BillWagner BillWagner added the doc-enhancement Improve the current content [org][type][category] label Aug 2, 2022
@dotnet-bot dotnet-bot removed the ⌚ Not Triaged Not triaged label Aug 2, 2022
@BillWagner
Copy link
Member

Hi @ArticadAlistair

I'd like to learn more about how you got to the state where you added a custom interpolated string handler in a class that needed mocking.

I hadn't considered the case where those would be combined. First, I expect that most .NET developers will use the interpolated string handlers provided by the .NET runtime. Second, I expected that custom handlers would only be created for performance reasons where not creating the output string, or truncating the output could save important machine cycles, I'd like to know more about what I missed in that analysis.

@ArticadAlistair
Copy link
Author

ArticadAlistair commented Aug 2, 2022 via email

@IEvangelist
Copy link
Member

If you're using the ILogger (or ILogger<T>) abstractions, you should consider using the NullLogger.Instance for unit testing. For more information, see Logging in .NET.

@ArticadAlistair
Copy link
Author

I've written my own interface. (When I read up on what's already out there, I found it somewhat overwhelming, and yet nothing seemed to do exactly what I needed.) But in any case, my unit tests of classes that use logging needed to verify that error conditions had been communicated to the logger, so a null logger wouldn't really help with that.

@IEvangelist
Copy link
Member

I didn't realize that this was related to this tutorial:

https://docs.microsoft.com/dotnet/csharp/whats-new/tutorials/interpolated-string-handler

This isn't an official runtime type, it's all custom, and this article doesn't discuss unit testing. I'm not so sure that this is really an issue at all, but I'll defer to @BillWagner.

@ArticadAlistair
Copy link
Author

ArticadAlistair commented Aug 4, 2022

My point is that by not discussing unit testing, anyone doing Test Driven Development and incorporating ideas from this tutorial is in for a potentially nasty surprise, unless they are fully aware of a mocking framework's inability to mock methods with ref struct parameters. I'm guessing not many developers are aware of that. I think the article should carry at minimum a one-sentence warning about this pitfall.

@BillWagner BillWagner self-assigned this Nov 22, 2024
@BillWagner BillWagner added the 🗺️ reQUEST Triggers an issue to be imported into Quest. label Nov 22, 2024
@dotnetrepoman dotnetrepoman bot added 🗺️ mapQUEST Only used as a way to mark an issue as updated for quest. RepoMan should instantly remove it. and removed 🗺️ mapQUEST Only used as a way to mark an issue as updated for quest. RepoMan should instantly remove it. labels Nov 22, 2024
@BillWagner BillWagner moved this from 🔖 Ready to 👀 In review in dotnet/docs November 2024 sprint Nov 22, 2024
@BillWagner BillWagner moved this from 👀 In review to 🏗 In progress in dotnet/docs November 2024 sprint Nov 22, 2024
@BillWagner BillWagner linked a pull request Nov 22, 2024 that will close this issue
@dotnet-policy-service dotnet-policy-service bot added the in-pr This issue will be closed (fixed) by an active pull request. label Nov 22, 2024
@dotnetrepoman dotnetrepoman bot added 🗺️ mapQUEST Only used as a way to mark an issue as updated for quest. RepoMan should instantly remove it. and removed 🗺️ mapQUEST Only used as a way to mark an issue as updated for quest. RepoMan should instantly remove it. labels Nov 22, 2024
@sequestor sequestor bot added 📌 seQUESTered Identifies that an issue has been imported into Quest. and removed 🗺️ reQUEST Triggers an issue to be imported into Quest. labels Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-enhancement Improve the current content [org][type][category] dotnet-csharp/svc in-pr This issue will be closed (fixed) by an active pull request. Pri2 📌 seQUESTered Identifies that an issue has been imported into Quest. whats-new/subsvc
Projects
Status: 👀 In review
Development

Successfully merging a pull request may close this issue.

5 participants