You can delete FileAlignment and AutoGenerateBindingRedirects as well.

I think it would look better to keep the explicit list of files here.

This looks like cruft generated by the C# compiler in debug builds. It should not be here. This should have just what is needed:

{ 
   .maxstack  2 
   ldarg.1 
   ldarg.2 
   add 
   ret
}

I think this should be [Theory], the test data should be auto-generated by enumerating all methods in the test file, and filtering the ones for given theory based on the test name conventions - e.g. one theory should get all valid tests and other theory should get all invalid tests. For invalid tests, you can also have convention to encode the expected error in the test name.

You should be able to get the test files dropped to the output by referencing the Project:

https://blogs.msdn.microsoft.com/kirillosenkov/2015/04/04/how-to-have-a-project-reference-without-referencing-the-actual-binary/

Could you please share more about this? Currently there are only two projects: 1 ILVerify, 2 ILVerify.Tests, which contains the xunit code, and the .il file is just in a subfolder. (to get the dlls, which we test on I call ilasm manually). Should I move the .il files into a separate project and do the referencing trick with that project? I’m trying to do that, but I have problems with creating the .ilproj based project. I did not find that in VS, so I tried to do it manually (based on src\ILCompiler.TypeSystem\tests\ILTestAssembly\ILTestAssembly.ilproj), but VS cannot open the project I created manually (ILVerify.Tests.ILTests.ilproj : error : Project file is incomplete. Expected imports are missing.). So is this the way I should do it? If yes, is there a documentation on .ilproj? (google does not really help..).

.ilproj is not supported by the .NET SDK out of the box, so there is no mainstream documentation for it. .NET SDK includes targets based on the project extension. For example, you can find code like this in Sdk.targets from the .NET SDK:

    <LanguageTargets Condition="'$(MSBuildProjectExtension)' == '.csproj'">$(MSBuildToolsPath)\Microsoft.CSharp.targets</LanguageTargets>
    <LanguageTargets Condition="'$(MSBuildProjectExtension)' == '.vbproj'">$(MSBuildToolsPath)\Microsoft.VisualBasic.targets</LanguageTargets>

Notice that there is no condition for .ilproj. You have to take of the msbuild targets for .ilproj yourself. I assume that it is what the VS is complaining about.

The .ilproj files work in the CoreRT build today because of our own buildtools extension that setups the targets for .ilproj: https://github.com/dotnet/buildtools/blob/03988127f489bcd1cf58d71131ac628620d53de9/src/Microsoft.DotNet.Build.Tasks/PackageFiles/FrameworkTargeting.targets#L106

I doubt that you would be able to include the buildtools ilproj targets into your project because of your have started using latest .NET CLI.

I think it should be ok to depend on manual building for now, and add the project file once #3682 is fixed.

Ok, if I update the readme file I will document this, and then I will revisit this when #3682 is done. Thanks for the explanation though!

Is there something better than this? According to what I found the method feeding MemberData must return IEnumerable<Object[]>, but what we need is just IEnumrable<TestCase>

I think it is ok. An alternative would be to be just pass the individual elements as arguments to the test method.

If you keep the custom TestCase type, you may want to implemented IXunitSerializable on it to make it work well with XUnit - to get the test accounting right amount other things (https://stackoverflow.com/questions/30574322/xunit-memberdata-tests-show-up-as-one-test-instead-of-many).

It would be nice if the tests are using just the public surface, no internals.

Would it make sense for the test to generate the method tokens to verify and pass them to the verification method in the ILVerify?

It should avoid the need to have access to the CoreRT type system from the test driver; and it will work towards a .dll flavor of the verifier.

Agree, that would be nice and I’m sure we want to have a .dll flavor of ILVerify. I wanted to test without InternalsVisibleTo, but currently there is no API surface, everything is internal. So I think, the first step is to define what we want to be public. I think we need VerifierError, VerificationErrorArgs, and most importantly some methods which are currently on ILImporter (or we create a façade class for that?).

Do we want to do that here? Or create an issue, define it there and I do it in another PR.

The ILImporter (and everything from the CoreRT type system under Common) should stay internal. There should be a public façade for the few pieces that need to be public.

I am fine with creating an issue about defining what we want to be public, and switching tests to run against this surface later.

This should be System.Runtime given the other changes.

The comment does not match the method name - I do not think we need this comment.

This can be just:

ldstr "sfdsf"
ldc.i4.3
add
ret

If you are generating the test cases by compiling C# and then running ildasm on it - compile the C# with /o+ to avoid the debug-only cruft.

Could you please add the license header to this file

Internal.IL.LocalVerificationException looks too verbose. Do we really need to differentiate the exception type here?

Delete comment.

Delete comments.

It may be better to use typeof(Object).Assembly here. It will make the tests work on both CoreCLR where the system module is System.Private.CoreLib, and desktop where the system module is mscorlib.

I think this can be launchSettings.json so that it applied globally.

Nit: names of locals should not start with _

CoreTestAssembly -> System.Runtime

The System.Runtime should still be here like before (get it via Assembly.Load).

Constructing ResourceManager is expensive. It should be done only once.

This catch all should not be needed.

ILReader does not match what this type does - it does not read IL.

Would it make sense to merge it with TestDataLoader ?

Yes, merged.

Is this used anywhere?

I think it is ok. An alternative would be to be just pass the individual elements as arguments to the test method.

If you keep the custom TestCase type, you may want to implemented IXunitSerializable on it to make it work well with XUnit - to get the test accounting right amount other things (https://stackoverflow.com/questions/30574322/xunit-memberdata-tests-show-up-as-one-test-instead-of-many).

The exceptions are kind of internal implementation detail. They are used to give up when things look too wrong to continue; and continuing would likely result in number of bogus errors to be generated. I am not sure whether it makes sense to test them directly.

It is related to the public surface discussion. I would expect that internal verifier exceptions would not be part of the public surface - they will be always caught internally.

they will be always caught internally

That sounds good, but the current implementation crashes with exceptions in many cases (e.g. with my first simple add test) and we have to handle this somehow in the test code in order to be able to add and run tests as soon as possible.

My suggestion: I remove this 4. part (so we ignore this in the name of the test methods) and I simply add an empty catch block in the test code to make sure the test method does not fail when ILVerify crashes. Once the
I would expect that internal verifier exceptions would not be part of the public surface - they will be always caught internally. is implemented we can remove the empty catch block

Sounds good.

Nit: var would be more appropriate here to fit with the rest

BTW: We try to follow https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md where possible. It says this about the var: We only use var when it's obvious what the variable type is (i.e. var stream = new FileStream(...) not var stream = OpenStandardInput()). I know that the ILVerify sources are not fully conformant with this style ... follow it when in doubts or for large chunks of new code.

Argument names should start with lower case.

Could you please open the issue on defining the public surface and add a link to it here?

Sure! #3734

This should be
typeSystemContext.SetSystemModule(typeSystemContext.GetModuleForSimpleName(systemRuntime.GetName().Name));

and not coreAssembly.GetName().Name.

right?

And System.Runtime forwards the "base" classes like System.Object both on full framework and on CoreCLR, so coreAssembly is either mscorlib or System.Private.CoreLib and we need that, because system.runtime depends on it. Is this assumption correct?

Either one will work in current implementation, but coreAssembly.GetName().Name is more correct. The system module is meant to be the actual module where the wellknowtypes live, not a forwarder.

Is this assumption correct?

Right.

Ok

This looks very fragile. Is there any way this can be done in the build? The type system tests (\src\ILCompiler.TypeSystem\tests\TypeSystem.Tests.csproj) simply have a ProjectReference to the test data assemblies with a few flags set that get MSBuild to deploy the test artifacts right next to the test binary.

This should be a temporary workaround. It should go away once this is added back to end-to-end build. Maybe add a comment about it?

Ok, added more comment. The discussion we had with @jkotas is here: #3725 (comment)

Nit: The CLR codebase tends to spell the word Metadata as MetaData for reasons unknown, but metadata is a legit word found in the dictionaries since the seventies or so. I've been avoiding importing that capitalization plague into CoreRT. We don't spell things like PlaceHolder, NotePad, UnderLine with this weird capitalization either.

Interesting insight. Ok, changed it to Metadata.

Nit: I would just make this enforce the capitalization too, for consistency's sake. No need for ToLower().