Winforms: Add InternalsVisibleTo to System.Windows.Forms.Design and System.Windows.Forms.Design.Editors

[Shyam-Gupta]

As per feedback received from @ericstj we should avoid using IVT even for test code.

[AdamYoblick]

I disagree with this, for various reasons. :)

But the most important reason is that we want our unit tests to exercise our code at the lowest level possible.

Imagine the scenario where you have a public method calling 5 internal methods. Imagine each of the internal methods has 3 code paths. That means, to get 100% code coverage, you have to write 15 unit tests for a single public function, and that's not including any other code paths in the public function.

It's much cleaner and more straightforward to test each internal method 3 times, exercising all the code paths of each one individually.

The argument that "we don't need to test internal methods" is not a good one, because the quality bar of our internal code should be the same as the bar for our public code.

Please don't remove the internals visible to, I'm very much against this.

[ericstj]

It's really up to your engineering team to decide this, but we do have a different policy in other dotnet repositories and you're welcome to research the background on that by reviewing those (heated) discussions in the other repositories.

[AdamYoblick]

But even if your team isn't testing internals across the board, what if I wanted to test one I just wrote and I want to make sure that test runs as part of the ci build?

I would think having the option to do so is more valuable than getting rid of that option because of a repo policy.

[ericstj]

I'm not going to defend this position as it is a bit of a philosophical debate. I just wanted to point out that winforms is taking a different path than other dotnet foundation repositories.

Here are some examples:
https://github.com/dotnet/corefx/issues/1604
https://github.com/dotnet/corefx/pull/29251#discussion_r183187480

@stephentoub or @jkotas might be able to state the position of CoreFx / CoreCLR better than I can. I couldn't find details in our contribution guidelines or other docs.

[jkotas]

I think this is up to project maintainers to decide. It is true that we do not use internals visible in core framework for reasons discussed in the linked issues. It is intentional choice made by the maintainers of the core framework.

The higher-level sub-projects in CoreFX repo are maintained by different folks, with different opinions, and they are happy to use InternalsVisibleTo, e.g.:

https://github.com/dotnet/corefx/blob/master/src/System.Reflection.Metadata/src/Properties/InternalsVisibleTo.cs
https://github.com/dotnet/corefx/blob/master/src/System.Numerics.Tensors/src/Properties/InternalsVisibleTo.cs

Number of other repos in CoreFX use InternalsVisibleTo. For example, the list of InternalsVisibleTo in Roslyn is so long that it does not even fit on a single screen.

I do not see a problem with WinForms using InternalsVisibleTo if the WinForms maintainers understand the trade-off and are happy to deal with the consequences.

[Tanya-Solyanik]

Wes' argument against friend assemblies: https://github.com/dotnet/corefx/issues/1604#issuecomment-98319086 He says that we'll have to re-visit our 3 clean internal tests from Adam's example each time we touch the implementation.

On the other hand we have internal types that are exposed to windows via native APIs (AccessibleObjects associated with internal types) and thus are part of our public contract.... being able to new them up would be really sweet, compared to introducing a custom accessibility client.

@Shyam-Gupta how many existing tests in System.Windows.Forms are affected?

[zsd4yr]

Is there any reason why we would actively prevent people from writing tests?

I understand the concern of testing internals without publics when public-facing surface area is our greatest level of concern, but I don't think anyone is suggesting that testing internals is a substitute for testing publics?

As Jan and Eric have indicated, this is really a philosophical debate about the breath of our testing and whether or not we should (somewhat artificially) restrict the domain of our tests and the ability for community members to write tests. Maybe we should have a brief meeting about this? --To discuss the trade-offs of each approach?

[jkotas]

(somewhat artificially) restrict the domain of our tests

InternalsVisibleTo does not imply that you have to restrict domain of your tests.

We do have occasional tests in corefx for core framework that test internals. They use private reflection instead of InternalsVisibleTo. Private reflection has extra barrier to entry, and thus it is used only as last resort. This setup without InternalsVisibleTo strongly encourages use the public APIs for as much testing as possible. As I have said, it is up to project maintainers to choose whether it is a worthwhile trade-off.

[Shyam-Gupta]

@Shyam-Gupta how many existing tests in System.Windows.Forms are affected?

@Tanya-Solyanik : ~100 tests are affected and will need to be fixedrewritten.

[AdamYoblick]

I just can't understand why we wouldn't want to be able to easily test internal methods in isolation. If engineers use this as a crutch to avoid testing publics, that's a different problem to solve. They don't have to be mutually exclusive.

Looking at how many internal tests exist right now is not a fair value comparison. What you should be looking at is the opportunity cost of not doing this. How many internal methods exist in the code base that will not be tested in isolation once we remove InternalsVisisbleTo? The answer is 2688, and that's just in System.Windows.Forms.dll.

[stephentoub]

As @jkotas stated, what the owners of the winforms project decide is up to them.

For corefx, though, I enumerated various reasons we don't like InternalsVisibleTo at https://github.com/dotnet/corefx/pull/29251#discussion_r183211946, and it includes actually making the binaries that we ship worse. There are some times when we want to test internals, and we do that in one of two ways:

• Dedicated unit test rather than functional test projects that include the relevant product source files. If the functionality is testable on its own, that means it can stand on its own, and so it should be fine to include the source in such projects. We do this in particular in some of our networking projects. This also makes it very explicit what is testable and what isn't.
• Private reflection as an absolute last resort: I could probably count on one hand the number of times we do this, though.

[zsd4yr]

Discussed within the internal team today. We are not removing this and instead allowing it to be added to other binaries. Testing publics is the ciritcal, yet we should also allow testing internals. Changing the issue name to correspond with the new strategy.