An idea for making private code more easily testable

[athomas-octoberthree]

Greetings, current accessibility levels for members is great for object-oriented design, but poor for testing. I propose a new attribute that keeps the advantage of the private accessibility level in terms of object-oriented design, but allows visibility for testing. And, this would mimic internal in terms of visibility for testing, but allows for the benefits of private that internal does not allow.

The naming is up for discussion, but for purposes of discussion I'll call the attribute TestableAttribute. The general way it would work is as follows:

1. In a class, create members for functionality.
2. Define member access according to familiar standards: public for API exposure, internal for API exposure within an assembly, and private for supporting methods.
3. Some supporting methods should be testable as standalone, but are not appropriate to expose through an API for standalone consumption. Add the [Testable] attribute to these methods.
4. This would enable these private methods to be called from unit tests.

This provides the following benefits:

1. Code not suitable for direct consumption can be tested in-place.
2. Shenanigans to shoehorn code into a visible member via test-specific wrappers etc is no longer required.
3. Private code that is meant to stay private, not exposed outside the class, remain as-is with no changes required.
4. Internal code, meant to be consumed in the same assembly but not meant to be private to the class, can be kept with the original intent.
5. Private code that has been made internal, in an effort to make something that would otherwise be private exposed, can once again be restored to the obvious accessibility level.
6. Backwards-compatibility with all pre-existing C# code.
7. Easy to understand the intent.
8. Easy to consume and use the new attribute.
9. Very little to no room for abusing the new attribute.

I can also imagine the following downsides, which I raise because obviously I don't think they come near to outweighing the benefits:

1. Difficulty in getting tooling (Roslyn, IDEs, etc) to play nice with this.
2. People may point out this is - functionally speaking - just internal with a different name. To which I say first a different name is a benefit here; second that depending on how this is implemented this has the possibility to have some other exciting differences from internal and private.

Thank you for your consideration, I look forward to the discussion.

[HaloFour]

That attribute already exists for internal members: InternalsVisibleToAttribute

[HaloFour]

This proposal is for a very specific scope: unit testing. Not the related but more broad concept of opening up visibility to other assemblies. It might seem unwise to propose a change targeting such a narrow scope, but unit testing affects practically every domain that C# is used in.

That usage is already explicitly covered by the existing solution. The runtime doesn't need to add support for a new set of attributes to indicate why you want to relax that accessibility, that can be achieved through comments.

[athomas-octoberthree]

That usage is already explicitly covered by the existing solution...

Well, I humbly disagree. First, it is not explicit. The documentation for InternalsVisibleToAttribute doesn't mention the phrases "test", "unit testing", or the like anywhere. Of course in reality it can be and usually is used for unit testing. But it is not explicit. It's adopted because there has been nothing better. Which brings me to my second point of disagreement: that usage is covered as you say, but it is only covered poorly.

[athomas-octoberthree]

(neither do the supplemental remarks mention "test", "unit testing", or the like)

[HaloFour]

The documentation for InternalsVisibleToAttribute doesn't mention the phrases "test", "unit testing", or the like anywhere.

That sounds like it should be addressed by documentation. Testing internals is the primary reason as to why that attribute was added and runtime has explicit support for it, although it's certainly not limited to that use case.

[colejohnson66]

If you really need access to something deep inside your testing target, you should consider rearchitecting. But if not, reflection and UnsafeAccessorAttribute are your friend.

[ufcpp]

What about using UnsafeAccessor and Source Generator e.g. PrivateProxy?

[athomas-octoberthree]

Could be an option for implementation (a way to make my proposal work). But what developer, having to choose between my proposal or setting up something PrivateProxy-like, would not prefer my proposal if it were available?

Also, UnsafeAccessor has the same downsides to just making a method internal instead of private: it is unclear that it is specifically for, and recommended for, unit testing private code.

[ufcpp]

Maintenance costs for language features are heavy, and the hurdle for adding new features is extremely high. What can be achieved using a source generator is rarely made into a language feature.

[CyrusNajmabadi]

it is unclear that it is specifically for, and recommended for, unit testing private code.

I'm not really sure what the proposal is even asking for. :)

Some supporting methods should be testable as standalone, but are not appropriate to expose through an API for standalone consumption. Add the [Testable] attribute to these methods.

Ok. You can already add such an attribute today. Not sure what the language would have to do with this.

This would enable these private methods to be called from unit tests.

How would it do that? What does this actually do at the end of the day? How is that accomplished? To start, it's unclear to me how a 'unit test' would even be identified. Currently, from the lang/runtime's perspective, it's "just code". It just happens that there are many different libraries out there for helping people write tests (in many different ways).

But there's nothing that distinguishes these libs from any other libs (except that a few use names like 'unit' in their package name). Otherwise... it's just code :)

So it's unclear what this would actually do and how it would actually flow out from lang to ecosystem.

[DavidArno]

To my mind, you missed the biggest downside to this idea: private functions are implementation details and therefore should never be tested directly. Therefore any idea that would make it easier to directly test a private functions is inherently bad practice in my view.

If you have code that needs directly testing, then that code is part of the public API of that class/library as it has expected behaviour. Therefore mark it public. If you for some reason absolutely do not want it public but absolute do want to be able to test directly eg for debugging purposes, then make it internal and use InternalsVisibleTo to gain access to those things.

[athomas-octoberthree]

Thank you for the patient and even-handed replies. Philosophically I'm actually in agreement with you: "The tests are a demonstration that your actual consumers will all be ok." "...hiding away implementation details to allow them to change whenever needed, without effecting consumers (including tests), is a fundamentally good idea." Yes and yes - philosophically. But practically, I do not like the idea of being pigeonholed into structuring code so that after it is set up well for consumers and maintenance, I then must change things so that unit testing can take place by only interacting with the public/internal aspects.

(btw not against unit testing via, or even primarily via, public/internal: "real surface area wasn't being tested, but instead the test harness was poking into the internals and thinking things were good because of the checks it was doing on that." imo the mistake in that situation was not poking into the internals, but forgetting to test the part that is actually interacted with.)

But, the massive amount of experience behind your comments makes me back down. You've seen this proven in the trenches. So as someone else in the trenches for years but with less experience, I'll bow out of this, and trust that in time I'll see the light. Thank you again, it was a pleasure to have this conversation.

[CyrusNajmabadi]

But practically, I do not like the idea of being pigeonholed into structuring code so that after it is set up well for consumers and maintenance, I then must change things

You are not. :)

It is possible today to have internal details which tests can test. Many projects do this (including roslyn).

Our general point though is: since the problem is already solved, creating a new feature, which is identical to the existing feature, to solve it again, isn't a good thing :)

But, the massive amount of experience behind your comments makes me back down.

Note: i wouldn't back down due to experience. But if the points here address the main concerns you have or not. Afaict, they do in that they talk about best practices for testing while also showing how your scenario can be covered with existing tech if you choose to use that tech.

[HaloFour]

I will note that the waters definitely get much murkier in those cases where you might, say, "inherit" a large monolithic codebase and are trying to retroactively carve it up and add tests. It's a bit of a chicken and egg situation too as you'd want to rely on the tests to aid in the refactoring but you need the refactoring to write the tests. Where everything is a single giant ball of code internal really isn't any different from public so bits of more isolated logic might get split out into private members to try to prevent them from being used directly. Custom analyzers can help with that now, in the past I might rely on conditional compilation to expose bits of surface for the purposes of testing.

Anyway that's not a statement about this proposal, just that reality is often a lot messier than we'd want it to be and following idealistic practices are sometimes not an option.

I do think that IVTs could use a bit of an overhaul. I don't like how they're all or none. It'd be very nice if it could be much more modular, e.g. exposing specific internal types to an external assembly.

[athomas-octoberthree]

Thanks for the additional comments. If I could ask an additional question to help me understand your thinking, what would you say would be the purpose of "private" code, where private should be used instead of internal?

[CyrusNajmabadi]

private: Only this symbol sees it.
internal: Only symbols in my current assembly, or assemblies i explicitly grant trust to, can see it.

Note that a 'test lib' is exactly the sort of assembly i see a reasonable argument for granting trust to.

[jrmoreno1]

I would say the typical solution today is protected internal combined with internalsvisibleto attribute, instead of asking for something entirely different, I would suggest that if any change were to be made, it would be to allow private internal. Which restricts the scope of internal, instead of protected internal which expands the scope of protected. Because all we really want is the attribute.

[timcassell]

We have private protected.