Repository navigation
MonoTests.System.Reflection.Emit.SaveTest.Save test fails in Release configuration with BCL linking on #1186
Description
A test from the Mono BCL test suite (MonoTests.System.Reflection.Emit.SaveTest.Save) fails when the tests are built in Release mode with linking set to SdkOnly.
It seems that the linker somehow changes the order of interfaces in emitted code since turning BCL linking off makes the test work again. The test also works fine in Debug configuration (with the linker off)
Steps to Reproduce
- Comment out this line
- Run
make CONFIGURATION=Release run-apk-testsfrom the top Xamarin.Android dir - Wait patiently
- Find the failure in the NUnit tests result file (
TestResult-Xamarin.Android.Bcl_Tests.nunit-Release.xmlin the top Xamarin.Android directory)
Expected Behavior
The test should run fine with BCL linking on
Actual Behavior
The test fails when executed with BCL linking on
Version Information
Xamarin.Android/master (c4e81655ac22b06806e732e9dd0bc729e8d2c1d7)
Test failure
MESSAGE:
Expected: <iface1>
But was: <System.IComparable>
+++++++++++++++++++
STACK TRACE:
at MonoTests.System.Reflection.Emit.SaveTest.CheckAssembly (System.Reflection.Assembly a) [0x000a0] in <1e30aff9165c427db5bedc42c5906861>:0
at MonoTests.System.Reflection.Emit.SaveTest.Save () [0x00b6d] in <1e30aff9165c427db5bedc42c5906861>:0
at (wrapper managed-to-native) System.Reflection.MonoMethod.InternalInvoke(System.Reflection.MonoMethod,object,object[],System.Exception&)
at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00032] in <0a3e31ed05774b07957e0b91c5cab789>:0
Activity
radekdoulik commentedon Jan 16, 2018
This doesn't look like a linker bug, as the assembly is created by the test itself, not by the linker.
jonpryor commentedon Jan 16, 2018
@radekdoulik: While that is indeed the case, it's only with a linked BCL that the problem occurs. Running the test with a Debug/unlinked BCL results in the test passing.
We don't understand why or how linking would be involved, but it's the straw we're grasping at.
radekdoulik commentedon Jan 16, 2018
Further looking into that, I have put the code from the test to simple android app and it works OK in the Release configuration. The first interface is
iface1.So not sure yet what might be broken here.
radekdoulik commentedon Jan 16, 2018
OK, the issue is the test is wrong :-)
In the documentation it is clearly stated, that the interfaces are not returned in any particular order. https://msdn.microsoft.com/en-us/library/system.type.getinterfaces(v=vs.110).aspx#Anchor_2
grendello commentedon Jan 16, 2018
@radekdoulik yes, it's not documented but @marek-safar stated that the runtime team aim for 100% compatibility with .NET, and the ordering tested for is what .NET does. As @jonpryor said, we're clutching at straws here as the linking was the only real difference between the builds and it is consistently reproducible.
radekdoulik commentedon Jan 16, 2018
@grendello not sure I understand you here, when you say that it is not documented . The documentation says
radekdoulik commentedon Jan 16, 2018
@marek-safar Do you have link to our current
System.Type::GetInterfacesimplementation at hand?grendello commentedon Jan 16, 2018
@radekdoulik By 'not documented' I meant it isn't stated that the order must be kept. The wording you pasted states that it 'must not' depend on the order, which would make the test invalid, but as @marek-safar stated we want to keep compatibility with .NET as there might be code out there which relies on the particular behavior of .NET (even if it isn't a documented one)
migueldeicaza commentedon Jan 16, 2018
You might just be getting lucky with some idioms/patterns in .NET, and might change with others, or might change across other implementations. The documentation is right, we should fix the test.
jonpryor commentedon Jan 17, 2018
On the implementation side of things, it appears that
RuntimeType.GetInterfaces()is implemented asves_icall_RuntimeType_GetInterfaces()which uses aGHashTableto contain the enumerated interfaces.I'm going to hazard a guess that
GHashTable/g_hash_table_insert()doesn't preserve insertion order, which means the docs are correct and should be followed, because there's no easy way of fixing the implementation, just the tests.marek-safar commentedon Jan 17, 2018
I'm ok with the test fix but we have bug reports like https://bugzilla.xamarin.com/show_bug.cgi?id=31020 and similar so you could actually end up fixing this issue in XA for real
[Tests] Do not assume order of returned interfaces
[Tests] Do not assume order of returned interfaces
[Tests] Do not assume order of returned interfaces
[Tests] Do not assume order of returned interfaces
[Tests] Do not assume order of returned interfaces
Bump mono to fix dotnet#1186
[Xamarin.Android.Build.Tests] Add support for testing on system appli…
[Xamarin.Android.Build.Tests] Add support for testing on system appli…