dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.58k stars 10.05k forks source link

Make BrowserFile testable #42690

Open linkdotnet opened 2 years ago

linkdotnet commented 2 years ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe the problem.

In its current state BrowserFile is an internal sealed class which makes it impossible to unit-test from the outside world.

Especially if you have functions like BrowserFileExtensions.RequestImageFileAsync which takes an IBrowserFile but throws exceptions when invoked with everything besides the concrete internal BrowserFile type.

Describe the solution you'd like

The easiest and least invasive solution would be to make the BrowserFile type public and not sealed.

Additional context

No response

TanayParikh commented 2 years ago

Thanks for contacting us @linkdotnet. Can you please tell us a bit more about what you'd like the test to look like / what you're attempting to test?

ghost commented 2 years ago

Hi @linkdotnet. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

SteveSandersonMS commented 2 years ago

To clarify, there are lots of places where the framework uses internal types and we surface a public interface for them. It's not desirable for all of those internal concrete types to become public, as that would undermine the idea of separating the implementation so the framework can evolve without every change being breaking.

In this particular case it sounds like what you really want to unit test is RequestImageFileAsync. Is that right? I understand this method is awkward in demanding you use a specific implementation of IBrowserFile. So I can understand wanting that to be made more flexible.

What I'm not sure of is what your unit test will actually do with RequestImageFileAsync. What do you want the test to assert? Could you give an example of a unit test you'd like to be able to write, so we can see what the assertions would be? Then maybe we can find some way to make the framework enable that kind of test. Thanks!

linkdotnet commented 2 years ago

To give a bit more background: We implemented some time ago an easy way to mock <InputFile> in bUnit (small example can be found here). This approach always reaches its limits exactly when the concrete type is required ... like in this case. If a user has production code which utilises RequestImageFileAsync then our test code will break.

From a Blazor point of view I do understand that virtually only BrowserFile will ever see that method. Also I very much agree that not all internal classes should be exposed.. just for the sake of complexity. That particular choice just seems odd from an external library maintainer point of view.

linkdotnet commented 2 years ago

Maybe a small amendment from my side. In this particular instance making RequestImageFileAsync work with the interface would be a good option as well. It just seems like more complexity.

In general it seems odd that I could implement my own implementation of lets say IBrowserFile which will sooner or later lead to runtime exceptions.

ghost commented 2 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.