NuGet / Home

Repo for NuGet Client issues
Other
1.49k stars 250 forks source link

Refactor `PackageSourceMapping` class to be Mockable #12825

Open donnie-msft opened 1 year ago

donnie-msft commented 1 year ago

The current implementation of NuGet.Configuration/PackageSourceMapping is not Mockable as defined here: https://github.com/moq/moq/wiki/FAQ#moq---non-overridable-members-may-not-be-used-in-setup--verification-expressions

There may be several changes needed to do so (source & tests). https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMapping.cs

For example Feedback in https://github.com/NuGet/NuGet.Client/pull/5355/files would require tests verifying # times the PackageSourceMapping object is referenced (eg, UIActionEngineTests) to now look at counts for particular method invocations (ie, GetConfiguredPackageSources). This isn't possible without making the class mockable.

nkolev92 commented 1 year ago

I don't think you need to mock it. It's a class with content, https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMapping.cs#L53.

Just create a new mapping.

donnie-msft commented 1 year ago

The current implementation is not Mockable as defined here: https://github.com/moq/moq/wiki/FAQ#moq---non-overridable-members-may-not-be-used-in-setup--verification-expressions

Currently, I'm testing # calls into the object, not the values of the object. Although, designing classes to be testable is a good practice regardless of what I'm doing right now.

nkolev92 commented 1 year ago

I'm sure there's more to it, but I'm not sure I see why this isn't testable. The linq you provided even says:

In this case there's no need to mock PagingOptions because it's easy to use a real one. Instead of this:

It's easy to create a new instance of PSM. Why is the number of calls relevant?

donnie-msft commented 10 months ago

There was a suggestion to test more of the utility methods in https://github.com/NuGet/NuGet.Client/pull/5459#discussion_r1372453094

nkolev92 commented 10 months ago

I don't really think it requires changes to this class. The logic is this class is not getting mocked, but the data and that's already easy to do.