Port DictionaryAdapter and tests to .Net Core #119

jeremymeng · 2015-12-04T07:30:12Z

DictionaryAdapter Xml feature and tests are excluded as they depend on Xml features that are not available on .Net Core.

All tests are passing. ~~Currently 1 test is failing. They need further investigation to either fix or disable for .Net Core~~
- Fixed ~~Some failed due to inaccessible members~~
- Fixed ~~Tests for dictionary proxy with prefixed keys~~
- Fixed ~~Resource tests failed. One is getting resource strings from .resx, the other is testing an embedded .txt file.~~
  - AssemblyResourceFactoryTestCase.CreateWithAbsolutePath
- Disabled as it requires PEVerify. ~~BaseTestCaseTestCase.TearDown_FindsVerificationErrors~~
- Disabled TraceLogger listener tests are configured in App.Config. ~~Need to find out how to do that for .Net Core.~~
  - TraceLoggerTests.FallUpToDefaultSource
  - TraceLoggerTests.FallUpToShorterSourceName
  - TraceLoggerTests.TracingErrorInformation
  - TraceLoggerTests.WritingToLoggerByType

@jonorossi

…T is defined.

…TER_XML is defined.

…an get rid of the converter shim for CoreClr.

…o investigate.

…sing in an Attribute type.

…ects. The file include [ClsCompliant(true)] which causes problem for the netcore project.

…GNAME.

jeremymeng · 2015-12-04T07:31:42Z

@jonorossi I need some help from you on the test failures, especially the first two types of failures. Thanks!

…of the resource path, while the desktop uses namespace.

…instead of PropertyInfo.ReflectedType which is not available on .Net Core.

…m.Configuration which is not supported on .Net Core.

jeremymeng · 2015-12-09T18:41:40Z

@jonorossi

I tried a hacky way (jeremymeng@0b4687c) to add trace listeners in the code since System.Configuration is not supported. But it does not match the expectation in TraceLogger.cs, specifically in Initialize() as it relies on configured trace sources: var foundSource = new TraceSource("Default", defaultLevel); at https://github.com/jeremymeng/Core/blob/0b4687cc8bf754dff6dd736bd9c467f6ffece93f/src/Castle.Core/Core/Logging/TraceLogger.cs#L153. So I disabled the tests instead.

Fixed ~~Now with only one test failure left. I will check whether embedding .txt file is supported on .Net Core. if you think it is not worth it I can just disable it.~~

Thanks!

jonorossi · 2015-12-14T05:22:27Z

README.md

@@ -42,13 +42,19 @@ Symbol                              | NET35              | NET40              |
 ----------------------------------- | ------------------ | ------------------ | ------------------ | ------------------
 `FEATURE_APPDOMAIN`                 | :white_check_mark: | :white_check_mark: | :white_check_mark: | :no_entry_sign:
 `FEATURE_ASSEMBLYBUILDER_SAVE`      | :white_check_mark: | :white_check_mark: | :white_check_mark: | :no_entry_sign:
+`FEATURE_BINDINGLIST`               | :white_check_mark: | :white_check_mark: | :white_check_mark: | :no_entry_sign:
+`FEATURE_CONFIGURATION`             | :white_check_mark: | :white_check_mark: | :white_check_mark: | :no_entry_sign:


There is already a FEATURE_SYSTEM_CONFIGURATION symbol.

jonorossi · 2015-12-14T05:33:50Z

@jeremymeng could you provide some detail about the missing XML features in .NET Core that DictionaryAdapter is using.

jonorossi · 2015-12-14T05:36:24Z

src/Castle.Core.Tests/Core.Tests/Resources/AssemblyResourceFactoryTestCase.cs

+#if !FEATURE_NETCORE_RESOURCE
+					new CustomUri("assembly://" + AssemblyName + "/CastleTests.Core.Tests.Resources.MoreRes.TestRes/content1")
+#else
+					new CustomUri("assembly://" + AssemblyName + "/Castle.Core.Tests.Core.Tests.Resources.MoreRes.TestRes/content1")


I thought you could set the name of the resource in the project.json file? If not, then I'd prefer we rename the .NET Framework one rather than more conditional compilation.

I thought you could set the name of the resource in the project.json file?

Yes, named resources are better here. Fixed.

jonorossi · 2015-12-14T06:05:37Z

src/Castle.Core/Components.DictionaryAdapter/DictionaryAdapterFactory.cs

@@ -171,7 +175,8 @@ private object InternalGetAdapter(Type type, IDictionary dictionary, PropertyDes
 		}

 		#region Type Builders
-
+
+#if FEATURE_APPDOMAIN


Since .NET 4.5 has AssemblyBuilder.DefineDynamicAssembly and AssemblyBuilder.DefineDynamicModule this should be FEATURE_LEGACY_REFLECTION_API instead so we can easily drop the old code once we no longer support pre-.NET4.5. Obviously the same applies to the calling code just above here.

jeremymeng · 2015-12-14T19:44:30Z

@jonorossi Thanks for the feedback! I will look into it today or tomorrow.

… method with #if FEATURE_ASSEMBLYBUILDER_SAVE ... #endif.

jeremymeng · 2015-12-15T00:52:27Z

@jonorossi The change to conditionally compile on IDataErrorInfo isn't too hard (jeremymeng@6f75c96). After the change, a lot of the *Validate* types/members won't make sense any more on .Net Core, as they depend on IDataErrorInfo. I don't know the usage of these any better than you do.

jeremymeng · 2015-12-15T01:14:16Z

@jeremymeng could you provide some detail about the missing XML features in .NET Core that DictionaryAdapter is using.

The major blocker here is the Xslt related stuffs (XsltContext)

jeremymeng · 2015-12-18T03:13:06Z

@jonorossi I've pushed the IDataErrorInfo change. The Xslt dependency seems non-trivial to exclude. Please let me know what you think.

jonorossi · 2015-12-23T05:31:09Z

@jeremymeng thanks for getting the feedback addressed. I've just done another review pass and this is what is left before we can merge:

Remove unneeded FEATURE_NETCORE_RESOURCE from README and project.json.
Remove unneeded FEATURE_NETCORE_COMPONENTMODEL_API from README and project.json.
Change exception message in DictionaryValidateGroup.IsValid to "IDataErrorInfo is not supported on this runtime." or something rather than mentioning .NET Core because other targets can use the same code in the future.
We need to work out something for excluding AssemblyInfo.cs in project.json, we've always had CLS compliant defined. I'd be happy to merge the PR without it excluded (like it is in the netcore branch) with the branch not compiling and then we can work out what to do next.

… symbols and using a better exception message.

jeremymeng · 2015-12-24T21:41:07Z

@jonorossi sounds great! I've pushed the suggested changes.

The ClsCompliant issue might be related to the newly introduced XUnit shim.

Port DictionaryAdapter and tests to .Net Core

jonorossi · 2015-12-29T04:22:02Z

Thanks, merged.

I think our next step should be getting the build going:
http://builds.castleproject.org/project.html?projectId=CoreNetCore&tab=projectOverview

Looking at the CLS compliant errors from it seems unrelated to xUnit.net, this is the first compiler warning:

'ProxyGenerator' cannot be marked as CLS-compliant because the assembly does not have a CLSCompliant attribute

Also the change you made to change the unit tests to using ValidationRuleAttribute rather than the interface has broken the .NET 3.5 unit tests, I haven't had a look at why that might be.

jeremymeng · 2015-12-29T06:28:51Z

@jonorossi Thanks!

Looking at the CLS compliant errors from it seems unrelated to xUnit.net, this is the first compiler warning:

This is a different issue than the one in the test project. I believe that both AssemblyInfo.cs files (in Castle.Core and Castle.Core.Tests projects) are generated by the non-CoreClr build so in dnx only build they don't exist yet. One possible solution is to check them in.

Also the change you made to change the unit tests to using ValidationRuleAttribute rather than the interface has broken the .NET 3.5 unit tests, I haven't had a look at why that might be.

I will take a look tomorrow.

jonorossi · 2015-12-29T06:32:01Z

@jeremymeng ah yes you are right, I remember realising that a few weeks back and started looking at how we'd run MSBuild to generate those files before running dnu. They can't be committed because they have the version number in them.

jeremymeng · 2015-12-29T18:42:50Z

They can't be committed because they have the version number in them.

If the files remain relatively stable (only change when version explicitly being bumped up), IMHO it is not that bad to check them in.

jonorossi · 2016-01-04T05:31:56Z

The problem is they don't remain stable across build configurations, I just mentioned version because it was the easiest to explain. The assembly info changes for each build configuration because it changes description (the build configuration is in there so you know how the assembly you've got was built), and a bunch of other security attributes.

If we commit the files then the files cannot ever be generated otherwise it'll show up in Git with unstaged changes pending when you are working on a different build configuration. I'm happy to look at committing it with conditional compilation (i.e. extending our CommonAssemblyInfo.cs) and we get rid of the MSBuild logic.

Since this issue is merged, lets start a new issue.

jeremymeng · 2016-01-04T07:23:59Z

ah, I now see how the file is generated.

I'm happy to look at committing it with conditional compilation (i.e. extending our CommonAssemblyInfo.cs)

I believe it is only the [ClsCompliant(true)] entry that is needed to fix the build break. Maybe just add that to CommonAssemblyInfo.cs?

jeremymeng added 16 commits November 22, 2015 12:51

Enable System.ComponentModel.BindingList only when FEATURE_BINDINGLIS…

13810ca

…T is defined.

Enable use of ListSortDescription when FEATURE_LISTSORT is defined.

b4e9bd7

Enable ISupportInitialize when FEATURE_ISUPPORTINITIALIZE is defined.

1f1e1b3

Enable DictionaryAdapter Xml feature only when FEATURE_DICTIONARYADAP…

123029d

…TER_XML is defined.

Add a shim for IDataErrorInfo for .Net Core.

c51e34e

Add FEATURE_DICTIONARYADAPTER_XML that was missed in previous commit.

69d4c7a

Merge DictionaryAdapter changes.

1e9b118

Replace System.Converter with a custom TestConverter delegate so we c…

d701b0f

…an get rid of the converter shim for CoreClr.

Enable DictionaryAdapter (sans Xml) features. Six new test failures t…

3e9bf3a

…o investigate.

Fix an error where GetCustomAttributes(attributeType, b) requires pas…

659ef82

…sing in an Attribute type.

Exclude AssemblyInfo.cs that is auto-generated by other non-Core proj…

25becef

…ects. The file include [ClsCompliant(true)] which causes problem for the netcore project.

Merge the change of disabling signing assebmlies.

6fdb436

Disable tests that need FEATURE_ASSEMBLYBUILDER_SAVE or FEATURE_STRON…

3de0b55

…GNAME.

Restore whitespaces that were removed by auto-formatting.

f10ae24

Remove comments that previously follows #if !SILVERLIGHT.

9c96e1b

Restore whitespace.

2f7bc72

jeremymeng added 5 commits December 7, 2015 16:45

Fix one assembly resource test. .Net Core uses assembly name as part …

2ce7a84

…of the resource path, while the desktop uses namespace.

Pass in the type on which the property is being reflected and use it …

87c36c3

…instead of PropertyInfo.ReflectedType which is not available on .Net Core.

Fix several tests on .Net Core as FEATURE_STRONGNAME is not supported.

8b23a0c

Disable a test that requires PEVerify on .Net Core.

3b67054

Disable tracelogger tests on .Net Core as they are depending on Syste…

a4ea30c

…m.Configuration which is not supported on .Net Core.

Fix another resource test due to the difference in resource path.

f9644dd

jonorossi reviewed Dec 14, 2015
View reviewed changes

jeremymeng added 4 commits December 14, 2015 11:48

Remove empty "exclude" section.

4369d94

Use the already-defined FEATURE_SYSTEM_CONFIGURATION constant.

1839d0c

Remove the Ignore attribute that no longer applies and wrap the whole…

8a7a18c

… method with #if FEATURE_ASSEMBLYBUILDER_SAVE ... #endif.

Use more proper FEATURE_LEGACY_REFLECTION_API constant.

04bc463

Use named resources.

8f1e4b3

jonorossi mentioned this pull request Dec 17, 2015

Support for .NET core #90

Closed

jeremymeng added 2 commits December 17, 2015 19:00

Use FEATURE_IDATAERRORINFO conditional compilation constant.

6f75c96

Merge branch 'IDataErrorInfo' into netcore

ef31f30

jeremymeng added 2 commits December 24, 2015 13:33

Address code review feedback: removing unused conditional compilation…

42d7c45

… symbols and using a better exception message.

Undo the exclusion of generated AssemblyInfo.cs.

ac6d6ee

jonorossi added a commit that referenced this pull request Dec 29, 2015

Merge pull request #119 from jeremymeng/netcore

7cec63e

Port DictionaryAdapter and tests to .Net Core

jonorossi merged commit 7cec63e into castleproject:netcore Dec 29, 2015

jonorossi added this to the v4.0 milestone Jan 4, 2016

jeremymeng mentioned this pull request Feb 11, 2016

Port unit tests to .NET Core Part 1 #118

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port DictionaryAdapter and tests to .Net Core #119

Port DictionaryAdapter and tests to .Net Core #119

jeremymeng commented Dec 4, 2015

jeremymeng commented Dec 4, 2015

jeremymeng commented Dec 9, 2015

jonorossi Dec 14, 2015

jonorossi commented Dec 14, 2015

jonorossi Dec 14, 2015

jeremymeng Dec 15, 2015

jonorossi Dec 14, 2015

jeremymeng commented Dec 14, 2015

jeremymeng commented Dec 15, 2015

jeremymeng commented Dec 15, 2015

jeremymeng commented Dec 18, 2015

jonorossi commented Dec 23, 2015

jeremymeng commented Dec 24, 2015

jonorossi commented Dec 29, 2015

jeremymeng commented Dec 29, 2015

jonorossi commented Dec 29, 2015

jeremymeng commented Dec 29, 2015

jonorossi commented Jan 4, 2016

jeremymeng commented Jan 4, 2016

Port DictionaryAdapter and tests to .Net Core #119

Port DictionaryAdapter and tests to .Net Core #119

Conversation

jeremymeng commented Dec 4, 2015

jeremymeng commented Dec 4, 2015

jeremymeng commented Dec 9, 2015

jonorossi Dec 14, 2015

Choose a reason for hiding this comment

jonorossi commented Dec 14, 2015

jonorossi Dec 14, 2015

Choose a reason for hiding this comment

jeremymeng Dec 15, 2015

Choose a reason for hiding this comment

jonorossi Dec 14, 2015

Choose a reason for hiding this comment

jeremymeng commented Dec 14, 2015

jeremymeng commented Dec 15, 2015

jeremymeng commented Dec 15, 2015

jeremymeng commented Dec 18, 2015

jonorossi commented Dec 23, 2015

jeremymeng commented Dec 24, 2015

jonorossi commented Dec 29, 2015

jeremymeng commented Dec 29, 2015

jonorossi commented Dec 29, 2015

jeremymeng commented Dec 29, 2015

jonorossi commented Jan 4, 2016

jeremymeng commented Jan 4, 2016