Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add some tests to System.Reflection.Emit - #10532

Merged
stephentoub merged 3 commits into
dotnet:masterfrom
hughbe:emit-tests
Aug 19, 2016
Merged

Add some tests to System.Reflection.Emit#10532
stephentoub merged 3 commits into
dotnet:masterfrom
hughbe:emit-tests

Conversation

@hughbe

@hughbe hughbe commented Aug 2, 2016

Copy link
Copy Markdown

Tests that were deleted (e.g. for EnumBuilder, TypeBuilder) have been moved to a different file (e.g ModuleBuilderDefineEnum or ModuleBuilderDefineType)

  • DefineDynamicAssembly
  • DefineDynamicModule
  • DefineDynamicType
  • DefineDynamicEnum
  • {G|S}etCustomAttribute

- DefineDynamicAssembly
- DefineDynamicModule
- DefineDynamicType
- DefineDynamicEnum
- {G|S}etCustomAttribute
@hughbe

hughbe commented Aug 3, 2016

Copy link
Copy Markdown
Author

/cc @stephentoub @atsushikan

I'm a little confused as to why the non-Windows builds are failing - any ideas would be appreciated!

@ghost

ghost commented Aug 3, 2016

Copy link
Copy Markdown

I've been pulled off to some crit-path stuff outside of x-plat so I really can't devote time to test reorganizations right now. Sorry.

@hughbe

hughbe commented Aug 3, 2016

Copy link
Copy Markdown
Author

Don't worry!


Assert.Equal(1, assembly.Modules.Count());
Module module = assembly.Modules.First();
Assert.Equal("<In Memory Module>", module.Name);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this module name defined? I'm wondering if this string is coming from the product source and might be localized such that this could fail in non-English builds.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's defined here: https://github.com/dotnet/coreclr/blob/master/src/dlls/mscorrc/mscorrc.common.rc

I think it is localisable, so I've changed it to Assert.NotEmpty instead

@stephentoub

Copy link
Copy Markdown
Member

@dotnet-bot test this please

@stephentoub

Copy link
Copy Markdown
Member

One comment/question, otherwise LGTM. Thanks.

@hughbe

hughbe commented Aug 16, 2016

Copy link
Copy Markdown
Author

@stephentoub Thank's for the review, but what about the failing non-windows builds?

@stephentoub

Copy link
Copy Markdown
Member

what about the failing non-windows builds?

It looks like it's having trouble with defining/loading types that have special Unicode characters in them. Have you tried narrowing down to exactly which inputs are causing the problem? Presumably it's "\uD800\uDC00" in the type name?

@hughbe

hughbe commented Aug 19, 2016

Copy link
Copy Markdown
Author

@stephentoub thanks.

I've taken a look, it seems that ModuleBuilder.GetType doesn't work with "\uD800\uDC00" when the ignoreCase parameter is set to true.
I've filed issue #10989 to fix this. Unfortunately, I've had to comment out the code, instead of wrapping it in if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) because I get a build error saying "One or more packages are incompatible with .NETCoreApp,Version=1.1" when I add a reference in project.json for "System.Runtime.InteropServices": "4.0.20",

@stephentoub

Copy link
Copy Markdown
Member

Thanks for investigating, @hughbe.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants