Refactoring the NuGet XPlat CLI reports to be testable

[drewgillies]

I'll raise this as a discussion as there may be a better way to do this. I prototyped some changes to the XPlat list package command runner code to make it more directly testable in UTs. The current state of the code is:

• do something
• write to console in real time
• do something else, maybe directly in the file system this time
• write to console in real time
• do something else, maybe by fetching metadata from a service
• write to console in real time
• etc...

This should have a report writer type and file system types and metadata fetch types which can be abstracted away for testing. Here's the prototype. It doesn't include proposed fixtures or tests, but these can be extrapolated and mocked/faked from the interfaces presented here.

Thoughts?

https://github.com/NuGet/NuGet.Client/tree/dev-drewgil-refactorforreporttesting

[drewgillies]

/cc @zivkan @nkolev92 @dominoFire @joelverhagen @aortiz-msft

[nkolev92]

I definitely like the general idea.
Didn't dig deep in the PR, but have some of sort of logging abstraction instead of using Console.WriteLine is necessary imo.

We should consider this change early in the next release imo, that way it gives us plenty of time to catch potential issues.

[zivkan]

My preference is that rather than abstracting out the logging/output of messages, we should separate the business logic into "1. create a data model with all the data, but do zero output, and 2. do output, with zero business logic". Basically MVC for the command line.

We have an old request to add a --format json option, which would be much easier, in my opinion, if output was separated from gathering data, compared to just abstracting out the output methods, like the proposed branch is currently doing.

[drewgillies]

I considered this idea too--but this would mean output latency. Users would need to wait for the whole command to execute before anything is shown. However, you may consider this an appropriate change in behavior.

[zivkan]

Regarding an abstraction for metadata fetching, NuGet.Protocol's SourceRepository already supports a kind of mocking. It has extensibility with its constructor and GetResourceAsync<> methods to allow the caller to provide its own resource providers with higher precedence than the built-in resource providers. We already have tests in the code that use this to provide a SourceRepository object with mocked/hard-coded results that doesn't need to really hit the network.

But I'm not opposed to having a simpler API to mock for tests that's more specific to what dotnet list package uses. It's non-trivial to create a mock SourceRepository. Just saying something already exists if we want to re-use it.

[drewgillies]

It would be non-trivial on the surface, although from what I saw (and could have dug more) it looked like there would only be one or two direct method mocks to return testable data in the aggregated types, and the rest would just be mocked methods to call mocked methods on inner types. But that would be an exercise for the implementer, yes, and the option could be taken to use the model you describe.