search

nunit / nunit-console

NUnit Console runner and test engine
MIT License
215 stars 152 forks source link

Should we throw an exception when no service is available? #897

Open CharliePoole opened 3 years ago

CharliePoole commented 3 years ago

As I've been doing some work on removal of the UserSettings and RecentFiles services, I have noticed a problem with the API. When a user requests a service by calling GetService we return null if the service is not available. This was intended to allow flexibility when a particular service is not available. However, I find that it can lead to a lot of debugging when the return value is simply saved and the caller tries to use it at a later time.

Maybe for V4 we should throw an exception in this situation and supply a TryGetService method for those who deliberately choose the flexibility.

The interface we would have to change is IServiceLocator

mikkelbu commented 3 years ago

I haven't looked at the code, but I think the proposal makes sense.

ChrisMaddock commented 3 years ago

Yeah, this has caught me out before, I think it would be a sensible change. We'll probably need to improve our testing methods or introduce a workaround for cases where we currently create an engine without a full set of services. We could probably do with a utility method which creates an engine with a full set of Mock services?

CharliePoole commented 3 years ago

Most Unit testing uses a fake ServiceContext rather than a full engine. We add what we want to it, either real or fake services. Obviously the service we are testing has to be real, but all the others should be fake. My experience with UserSettings was that removing it triggered test failures but the tests were easy to fix once I knew where the error had arisen. The trouble was, finding that out wasn't easy because of the interface silently returning null.

If I were implementing this change, I'd just replace all the calls from one service to get another with the new try method right away. If there's already code that tests for null, it can be replaced with a test on the return value from TryGetService.

So that said, it may be possible to completely eliminate GetService rather than throwing an exception.

CharliePoole commented 3 years ago

Moved this to a feature and added the design label.

Remaining question is whether TryGetService should replace GetService or be added as another method to the interface. Could be resolved now or at the point where someone creates a PR.