Add a Functional.Tests project that doesn't have InternalsVisibleTo

[eerhardt]

Today, our ML.NET project has InternalsVisibleTo all of our tests projects. This is not ideal because we can't ensure that our public API meets scenarios that our customers can actually use. We could easily internalize something that is necessary to a scenario, but we wouldn't catch it by our tests because our tests all have internal-access.

We should add a new test project - or set of test projects - that do not have InternalsVisibleTo access. We can put all our public API tests in this project, and we can ensure that customers can use the same code to meet those scenarios. At first we should have at least one test in the project(s), and we can migrate and add new tests over time.

/cc @TomFinley @shauheen @glebuk

[eerhardt]

A direct example of the problem here is this conversation:

#2302 (review)

Here - the only API to enable this scenario was by using the new ValueMappingEstimator constructor. However, #2100 says we should internalize all estimator constructors. Using InternalsVisibleTo, the test can still get access to the internal API, but no customers would have been able to. There would be no way to create one of these estimators using the public API because there is no MLContext overload that provides it.

[abgoswam]

@eerhardt .. Just to clarify #2100 is for refactoring of the constructors

The internalizing of constructors is being done in #1798

[TomFinley]

I agree with this. So to give the example of #2100, part of that work would be that we'd make the MLContext driven mechanism then make the constructor internal, but how to know when that is done correctly if it is removed?

This was top of mind for me when doing #2300. While much of it was clearly stuff that didn't belong in any public surface, some of it was stuff that I wasn't quite sure we had an alternative for yet. (Some things I know we don't, like, e.g., filters, which AFAIK we in some cases still need to add per #933.)

At the same time, the coverage we get via accessing components our legacy (command line, IDataTransform, IDataLoader) or non-public-API (entry-points) mechanisms is unquestionably valuable. Also there are some things that are "useful"f or tests that we don't really want as part of our public API (ArrayDataViewBuilder is my posterchild for super-useful-for-tests-but-utterly-useless-for-API structures).

[sfilipi]

We can use this project for the Scenarios tests.

[rogancarr]

@eerhardt @TomFinley Question about this Test.Functional/ project.

I created the project, and I am migrating v1.0 Scenarios over to it. What I've noticed is that these aren't necessarily tests, as in "run these and make sure the APIs return the correct values". What they are, are compile-time checks that ask "Can I compose this in ML.NET?"

So my question for you two is, do we need non-InternalsVisibleTo tests or do we want to have compile-time checks for composition and put them in docs/Samples/? @sfilipi and I are favoring the latter.

[eerhardt]

My opinion is that we need non-InternalsVisibleTo tests. We need to ensure that customer scenarios work correctly, not just that they can compile. But we need to ensure they don't throw exceptions and they return the correct results.

[rogancarr]

Great. I'll keep them as tests then and add checks on the outputs. This will give them a baseline-y feel, which is nice.

Second question @eerhardt @TomFinley . The current tests folder defines a key that other projects can use to set InternalsVisibleTo. One thing that @sfilipi and I have been talking about is how to guarantee that nobody accidentally makes internals visible to these functional tests.

One solution is to move them to a different folder. This might have cascading changes, with scripts etc., so I'm disinclined from doing that.

Another solution is to have a test that uses reflection to try to compile code that we know doesn't work and verifies that it throws.

We can also not worry about this. What do you two think?

[eerhardt]

One way we can try to avoid this is to ensure the new Functional.Tests assembly isn't strong-named. You can only add InternalsVisibleTo from a strong named assembly to other strong named assemblies.

So we could shut off these properties:

machinelearning/Directory.Build.props

Lines 101 to 106 in 30fe08a

	<!-- Signing properties -->
	<PropertyGroup>
	<AssemblyOriginatorKeyFile Condition="'$(AssemblyOriginatorKeyFile)' == ''">$(ToolsDir)Open.snk</AssemblyOriginatorKeyFile>
	<SignAssembly>true</SignAssembly>
	<PublicSign Condition="'$(OS)' != 'Windows_NT'">true</PublicSign>
	</PropertyGroup>

for this project. And then add a comment in the .csproj that says "we are turning off strong naming to ensure we never add InternalsVisibleTo for these tests".