Porting to .Net Core #617

Merged
merged 25 commits into from Jul 30, 2016

Conversation

Projects
None yet
8 participants
@jeremymeng
Collaborator

jeremymeng commented Feb 25, 2016

This is for #531.


Edited: 2016-7-20 by @jeremymeng

In current code the default AssemblyLoadContext is used instead of custom one.


Update: 2016-7-18 (UTC) by @blairconrad

We've for some time wanted to merge the coreclr branch to master and disable the netcore bits via a feature toggle, would be the inclusion/exclusion of that build from the nupkg.

I'm stealing the top of this "description" to list what I see as next steps that will achieve that goal as quickly as posssble. Note that these steps will not result in a releasable FakeItEasy with netcore support, but will make it so we're back to one active branch (master).

The primary goal is to preserve the current beaviour of the .NET 4.0 library while allowing use to finish our netcore-related work without interfering with .NET 4.0-targeted improvements.

Here's what I think should do before merging:

  • revert the 4.0 build to rely on Castle.Core 3.3.3 (I've pushed aa9f77e to my coreclr branch already - if it looks good, and @jeremymeng is willing, it can be cherry-picked)
  • update the Self-Initialized Fakes behaviour:
    • revert the 4.0 project's implementation to match the 2.2.0 release and
    • use #ifs and updated .csproj and projecxt.json files to exclude the Self-Initialized Fakes from the netcore project
  • split netcore and "regular" nuspecs
    • remove the 4.5 targetFramework from the nuspec(pushed c95ba3a to my coreclr branch)
    • remove the netcore build from the official nupkg and provide a second nuspec so we can continue to publish pre-release netcore packages from @jeremymeng's AppVeyor. ;-) (pushed ab528fd to my coreclr branch)
  • review usage of custom AssemblyLoadContext (I don't entirely understand this - I stole it from below)
  • General review - there are some typos and grammar that need fixing in comments, at least. I made comments in the PR already. Probably should've just pushed a commit. I may do that
  • Fix approve test - we have an [assembly: InternalsVisibleTo("FakeItEasy.Tests.dotnet, …] that is messing up the public API. One option is to adjust ApiApproval.ApproveApi.approved.txt, but I'd prefer to find a way to call the test assembly FakeItEasy.Tests.

And there are other things to do later, but that won't be required to merge to a single branch. It's probably best to create issues for them when they become relevant. Still, while they're in my head, we could eventually:

  • Add separate tests for integration tests, specs, and approval (I see @jeremymeng is already working on the integration tests)
  • Have the build system build all 4.0 and netcore versions.
    • And run tests on same.
    • As an interim step, we could add an optional TeamCity build to do as many of these as we have ready.
      Not something that would break the build (no need to scare off contributors), but something that will keep us on our toes.

As always, I probably missed something or made an error. Feel free to fix/add/remove/whatever. Thanks!


Update 06/28/2016:

1069 tests running on .NET Core, with 1 failure (same issue as castleproject/Core#184)

Update 06/22/2016:

1057 unit tests passing now on .NET Core comparing to 1088 on .NET framework v4.5. Disabled on .NET Core: 31 tests.

Update 06/21/2016:

Added a unit test project that targets netcoreapp1.0. Had to disable a bunch of tests (145) though, mostly due to missing of

  1. binary serialization
  2. ReflectedType (a lot of parameterized tests since ReflectedMethodData cannot be used on .NET Core)
  3. CurrentCultrue/CurrentUICultrue setter

Update 05/10/2016:
Replace binary serialization with Json serialization.

Update 04/12/2016:
The netcore project now builds fine and my Dnx test project only failed due to the lack of Binary serialization (currently `NotYetImplementedException is thrown for .NET Core).

still has one build error and I want to start a discussion about a couple of things, as well as getting feedback.

  • MethodInfo.ReflectedType isn't supported yet. This is the remaining build error. It is coming back (dotnet/corefx#5381), but in RTM. In the meantime, we need something. Options are: wrapping the MethodInfo with the original type, or passing the original type through the chain of method calls. NUnit did the former in their port (nunit/nunit#797).

    Edit: It looks that the MethodInfo wrapper is only needed for one file so I made the change. Once the ReflectedType becomes available in RTM this change can be reverted.

  • Replacement of BinaryFormatter? I've briefly tried DataContractSerializer but it seems unable to serialize MethodInfo.

  • is the usage of LibraryManager achieving the same goal as desktop version?

    Edit: I've changed to use LibraryManager.GetLibraries(). Tests passed. However it returns the transitive closure of the references, which is much more than number of loaded assemblies form AppDomain.GetAssemblies()

  • usage of custom AssemblyLoadContext needs review.

    Edit Now it's just AssemblyLoadContext.Default.LoadFromAssemblyPath()

Once the netcore project builds, things to do include, but not lmited to

  • Add a dnx test project that exercises the .Net Core code paths.

    Edit: added in a branch of my fork to test the porting changes. As there is a work item to migrate tests to xUnit. It make little sense to merge this back.

  • Add signing key to project.json.

@adamralph

/cc @thecodejunkie @jchannon

@jchannon

This comment has been minimized.

Show comment
Hide comment
@jchannon

jchannon Feb 25, 2016

For Nancy we replaced Binaryformatter with SimpleJson. It's one CS file
that you include in your project. Not sure if it works with MethodInfo
though. You'll have to try it

On Thursday, 25 February 2016, Jeremy Meng notifications@github.com wrote:

This is for #531 #531.
The netcore project still has one build error and I want to start a
discussion about a couple of things, as well as getting feedback.

  • MethodInfo.ReflectedType isn't supported yet. This is the remaining
    build error. It is coming back (dotnet/corefx#5381
    dotnet/corefx#5381), but in RTM. In the
    meantime, we need something. Options are: wrapping the MethodInfo with the
    original type, or passing the original type through the chain of method
    calls. NUnit did the former in their port (nunit/nunit#797
    nunit/nunit#797).
  • Replacement of BinaryFormatter? I've briefly tried
    DataContractSerializer but it seems unable to serialize MethodInfo.
  • is the usage of LibraryManager and custom AssemblyLoadContext
    achieving the same goal as desktop version?

Once the netcore project builds, things to do include, but not lmited to

  • Add a dnx test project that exercises the .Net Core code paths.
  • Add signing key to project.json.

@adamralph https://github.com/adamralph

/cc @thecodejunkie https://github.com/thecodejunkie @jchannon

https://github.com/jchannon

You can view, comment on, or merge this pull request online at:

#617
Commit Summary

  • Move FakeItEasy.csproj to FakeItEasy.Net40 folder and rename it to
    FakeItEasy.Net40.csproj.
  • Add FakeItEasy.dotnet project for .Net Core.
  • Enable serialization when FEATURE_SERIALIZATION is defined.
  • Add a pass-through Type.GetTypeInfo() extension method for legacy
    platforms.
  • Add a pass-through extension method GetMethodInfo() for Delegate
    type on legacy platforms.
  • Enable security permission feature only when
    FEATURE_SECURITY_PERMISSIONS is defined.
  • Add an extension method GetRuntimeInterfaceMap() for legacy
    platforms.
  • Introduce a conditional compilation constant
    FEATURE_REFLECTION_METADATATOKEN. Remove the usage of
    MethodInfo.MethodHandle.
  • Use LibraryManager to get referencing assemblies.
  • Add a custom AssemblyLoadContext to load assemblies from file paths.
  • Enable BinaryFormatter serialization only when FEATURE_SERIALIZATION
    is defined. The #else blocks still need implmentation.
  • Fix build errors and warnings.
  • Add a conditional compilation constant
    FEATURE_REFLECTION_GETASSEMBLIES for Net35, Net40, and Net45.

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#617.

For Nancy we replaced Binaryformatter with SimpleJson. It's one CS file
that you include in your project. Not sure if it works with MethodInfo
though. You'll have to try it

On Thursday, 25 February 2016, Jeremy Meng notifications@github.com wrote:

This is for #531 #531.
The netcore project still has one build error and I want to start a
discussion about a couple of things, as well as getting feedback.

  • MethodInfo.ReflectedType isn't supported yet. This is the remaining
    build error. It is coming back (dotnet/corefx#5381
    dotnet/corefx#5381), but in RTM. In the
    meantime, we need something. Options are: wrapping the MethodInfo with the
    original type, or passing the original type through the chain of method
    calls. NUnit did the former in their port (nunit/nunit#797
    nunit/nunit#797).
  • Replacement of BinaryFormatter? I've briefly tried
    DataContractSerializer but it seems unable to serialize MethodInfo.
  • is the usage of LibraryManager and custom AssemblyLoadContext
    achieving the same goal as desktop version?

Once the netcore project builds, things to do include, but not lmited to

  • Add a dnx test project that exercises the .Net Core code paths.
  • Add signing key to project.json.

@adamralph https://github.com/adamralph

/cc @thecodejunkie https://github.com/thecodejunkie @jchannon

https://github.com/jchannon

You can view, comment on, or merge this pull request online at:

#617
Commit Summary

  • Move FakeItEasy.csproj to FakeItEasy.Net40 folder and rename it to
    FakeItEasy.Net40.csproj.
  • Add FakeItEasy.dotnet project for .Net Core.
  • Enable serialization when FEATURE_SERIALIZATION is defined.
  • Add a pass-through Type.GetTypeInfo() extension method for legacy
    platforms.
  • Add a pass-through extension method GetMethodInfo() for Delegate
    type on legacy platforms.
  • Enable security permission feature only when
    FEATURE_SECURITY_PERMISSIONS is defined.
  • Add an extension method GetRuntimeInterfaceMap() for legacy
    platforms.
  • Introduce a conditional compilation constant
    FEATURE_REFLECTION_METADATATOKEN. Remove the usage of
    MethodInfo.MethodHandle.
  • Use LibraryManager to get referencing assemblies.
  • Add a custom AssemblyLoadContext to load assemblies from file paths.
  • Enable BinaryFormatter serialization only when FEATURE_SERIALIZATION
    is defined. The #else blocks still need implmentation.
  • Fix build errors and warnings.
  • Add a conditional compilation constant
    FEATURE_REFLECTION_GETASSEMBLIES for Net35, Net40, and Net45.

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#617.

+ var referencingLibraries = Microsoft.Extensions.PlatformAbstractions.PlatformServices.Default.LibraryManager.GetReferencingLibraries(fakeItEasyLibraryName);
+ return referencingLibraries
+ .SelectMany(info => info.Assemblies)
+ .Select(info => Assembly.Load(new AssemblyName(info.Name)))

This comment has been minimized.

@thecodejunkie

thecodejunkie Feb 25, 2016

info is already of the type AssemblyName so there is no need to explicitly create a new name.

@thecodejunkie

thecodejunkie Feb 25, 2016

info is already of the type AssemblyName so there is no need to explicitly create a new name.

@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie Feb 25, 2016

Is the usage of LibraryManager and custom AssemblyLoadContext achieving the same goal as desktop version?

No. Assembly.GetReferencingAssemblies gets you all the assemblies that Assembly is referencing. When you use GetReferencingAssemblies from the ILibraryManager you get the inverse, i.e all assemblies that references Assembly. You might have to use ILibraryManger.GetLibraries and iterate over them to build that graph yourself, using something like a HashSet<Assembly> to avoid duplicates

Is the usage of LibraryManager and custom AssemblyLoadContext achieving the same goal as desktop version?

No. Assembly.GetReferencingAssemblies gets you all the assemblies that Assembly is referencing. When you use GetReferencingAssemblies from the ILibraryManager you get the inverse, i.e all assemblies that references Assembly. You might have to use ILibraryManger.GetLibraries and iterate over them to build that graph yourself, using something like a HashSet<Assembly> to avoid duplicates

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Feb 25, 2016

Member

Wow, @jeremymeng thanks for this! 😲

Will take a closer look ASAP...

Member

adamralph commented Feb 25, 2016

Wow, @jeremymeng thanks for this! 😲

Will take a closer look ASAP...

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Feb 26, 2016

Member

Thanks for the PR @jeremymeng!

Replacement of BinaryFormatter? I've briefly tried DataContractSerializer but it seems unable to serialize MethodInfo.

I think we need serialization only for cross-AppDomain stuff (@FakeItEasy/owners, can you confirm?). Since .NET Core doesn't support AppDomains, we probably don't need to worry about that.

Member

thomaslevesque commented Feb 26, 2016

Thanks for the PR @jeremymeng!

Replacement of BinaryFormatter? I've briefly tried DataContractSerializer but it seems unable to serialize MethodInfo.

I think we need serialization only for cross-AppDomain stuff (@FakeItEasy/owners, can you confirm?). Since .NET Core doesn't support AppDomains, we probably don't need to worry about that.

@jeremymeng

This comment has been minimized.

Show comment
Hide comment
@jeremymeng

jeremymeng Feb 26, 2016

Collaborator

This one reads/writes files

private void WriteCallsToFile(IEnumerable<CallData> calls)
{
var formatter = new BinaryFormatter();
using (var file = this.fileSystem.Open(this.fileName, FileMode.Truncate))
{
formatter.Serialize(file, calls.ToArray());
}
}
private IEnumerable<CallData> LoadCallsFromFile()
{
var formatter = new BinaryFormatter();
using (var file = this.fileSystem.Open(this.fileName, FileMode.Open))
{
return (IEnumerable<CallData>)formatter.Deserialize(file);
}
}
.

Collaborator

jeremymeng commented Feb 26, 2016

This one reads/writes files

private void WriteCallsToFile(IEnumerable<CallData> calls)
{
var formatter = new BinaryFormatter();
using (var file = this.fileSystem.Open(this.fileName, FileMode.Truncate))
{
formatter.Serialize(file, calls.ToArray());
}
}
private IEnumerable<CallData> LoadCallsFromFile()
{
var formatter = new BinaryFormatter();
using (var file = this.fileSystem.Open(this.fileName, FileMode.Open))
{
return (IEnumerable<CallData>)formatter.Deserialize(file);
}
}
.

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Feb 26, 2016

Member

This one reads/writes files

private void WriteCallsToFile(IEnumerable<CallData> calls)
{
var formatter = new BinaryFormatter();
using (var file = this.fileSystem.Open(this.fileName, FileMode.Truncate))
{
formatter.Serialize(file, calls.ToArray());
}
}
private IEnumerable<CallData> LoadCallsFromFile()
{
var formatter = new BinaryFormatter();
using (var file = this.fileSystem.Open(this.fileName, FileMode.Open))
{
return (IEnumerable<CallData>)formatter.Deserialize(file);
}
}
.

Ah, yes... forgot about that.

Member

thomaslevesque commented Feb 26, 2016

This one reads/writes files

private void WriteCallsToFile(IEnumerable<CallData> calls)
{
var formatter = new BinaryFormatter();
using (var file = this.fileSystem.Open(this.fileName, FileMode.Truncate))
{
formatter.Serialize(file, calls.ToArray());
}
}
private IEnumerable<CallData> LoadCallsFromFile()
{
var formatter = new BinaryFormatter();
using (var file = this.fileSystem.Open(this.fileName, FileMode.Open))
{
return (IEnumerable<CallData>)formatter.Deserialize(file);
}
}
.

Ah, yes... forgot about that.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Feb 26, 2016

Member

Yeah, thanks, @jeremymeng. We really appreciate your work!

Member

blairconrad commented Feb 26, 2016

Yeah, thanks, @jeremymeng. We really appreciate your work!

@jeremymeng

This comment has been minimized.

Show comment
Hide comment
@jeremymeng

jeremymeng Feb 27, 2016

Collaborator

If anyone wants to build Castle.Core .NET Core compatible package:

  • Install Dnx RC1 update1 Instructions
  • git clone Castle.Core project and switch to netcore branch
  • In a command-line prompt, change directory to src\Castle.Core
  • dnu restore
  • dnu pack

If everything went smoothly you should get a Castle.Core.3.3.4.nupkg. Just copy it to your local NuGet source.

Note that in this PR I happened to use a different version 3.3.4-dev so you would need to either adjust the project.json of Castle.Core, or change it in FakeItEasy's project.json. Sorry for the trouble. Changed back to 3.3.4.

Collaborator

jeremymeng commented Feb 27, 2016

If anyone wants to build Castle.Core .NET Core compatible package:

  • Install Dnx RC1 update1 Instructions
  • git clone Castle.Core project and switch to netcore branch
  • In a command-line prompt, change directory to src\Castle.Core
  • dnu restore
  • dnu pack

If everything went smoothly you should get a Castle.Core.3.3.4.nupkg. Just copy it to your local NuGet source.

Note that in this PR I happened to use a different version 3.3.4-dev so you would need to either adjust the project.json of Castle.Core, or change it in FakeItEasy's project.json. Sorry for the trouble. Changed back to 3.3.4.

@NameOfTheDragon

This comment has been minimized.

Show comment
Hide comment
@NameOfTheDragon

NameOfTheDragon Mar 18, 2016

I'm so glad to see this is finally being worked on, thanks everyone for doing this. The lack of faking/mocking tools is such a massive hole in the CoreCLR space, affecting UWP and ASP.Net 5, and most of the tools depend on Castle Dynamic Proxy. This all means that unless you're prepared to use Microsoft's fork of MOQ, then unit testing is pretty much out of the question for those platforms. I know you know all this but I just wanted to reiterate it and let you know that a lot of people are going to be very grateful for CoreCLR compatibility :)

I'm so glad to see this is finally being worked on, thanks everyone for doing this. The lack of faking/mocking tools is such a massive hole in the CoreCLR space, affecting UWP and ASP.Net 5, and most of the tools depend on Castle Dynamic Proxy. This all means that unless you're prepared to use Microsoft's fork of MOQ, then unit testing is pretty much out of the question for those platforms. I know you know all this but I just wanted to reiterate it and let you know that a lot of people are going to be very grateful for CoreCLR compatibility :)

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Mar 31, 2016

Member

@jeremymeng I followed your instructions and I managed to build the coreclr branch.

One small thing - I had to edit the Castle.Core version to 3.3.4 in project.json in order to get the package built as 3.3.4. As it is in the repo, it builds as 0.0.0.

Member

adamralph commented Mar 31, 2016

@jeremymeng I followed your instructions and I managed to build the coreclr branch.

One small thing - I had to edit the Castle.Core version to 3.3.4 in project.json in order to get the package built as 3.3.4. As it is in the repo, it builds as 0.0.0.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Mar 31, 2016

Member

One small thing - I had to edit the Castle.Core version to 3.3.4 in project.json in order to get the package built as 3.3.4. As it is in the repo, it builds as 0.0.0.

@adamralph, @jeremymeng's instructions were valid when he wrote them. Since then, castleproject/Core#140 and castleproject/Core#153 were merged. The first provides a NETCORE target to the build so

build.cmd NETCORE

can be used to build the netcore branch of Castle.Core.

The second consolidates version numbers, but has the effect that if you don't build using something like the command above, you get 0.0.0.

Member

blairconrad commented Mar 31, 2016

One small thing - I had to edit the Castle.Core version to 3.3.4 in project.json in order to get the package built as 3.3.4. As it is in the repo, it builds as 0.0.0.

@adamralph, @jeremymeng's instructions were valid when he wrote them. Since then, castleproject/Core#140 and castleproject/Core#153 were merged. The first provides a NETCORE target to the build so

build.cmd NETCORE

can be used to build the netcore branch of Castle.Core.

The second consolidates version numbers, but has the effect that if you don't build using something like the command above, you get 0.0.0.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Mar 31, 2016

Member

Although I'm not sure why we aren't packing .NET Core yet. Let me get back to you on that.

Member

blairconrad commented Mar 31, 2016

Although I'm not sure why we aren't packing .NET Core yet. Let me get back to you on that.

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Mar 31, 2016

Member

@blairconrad I can't seem to get a nupkg out of just build.cmd either.

Member

adamralph commented Mar 31, 2016

@blairconrad I can't seem to get a nupkg out of just build.cmd either.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Mar 31, 2016

Member

@blairconrad I can't seem to get a nupkg out of just build.cmd either.

It's true. I should've said. But I'd forgotten.

It just builds. The packing step would have to be added. While you were making your comment, I was off rediscovering that and creating
castleproject/Core#171

Member

blairconrad commented Mar 31, 2016

@blairconrad I can't seem to get a nupkg out of just build.cmd either.

It's true. I should've said. But I'd forgotten.

It just builds. The packing step would have to be added. While you were making your comment, I was off rediscovering that and creating
castleproject/Core#171

@adamralph adamralph changed the title from [DO NOT MERGE] Porting to .Net Core to [WIP] Porting to .Net Core Apr 26, 2016

src/FakeItEasy/project.json
+ "../CommonAssemblyInfo.cs",
+ "../MethodInfoWrapper.cs"
+ ]
+}

This comment has been minimized.

@adamralph

adamralph May 11, 2016

Member

👀

@khellang khellang referenced this pull request in NancyFx/Nancy Jun 14, 2016

Merged

upgrade to FakeItEasy 2.0.0 #2488

4 of 4 tasks complete
@jeremymeng

This comment has been minimized.

Show comment
Hide comment
@jeremymeng

This comment has been minimized.

Show comment
Hide comment
@jeremymeng

jeremymeng Jun 21, 2016

Collaborator

Not sure how to get ReflectedMethodData working on .NET Core, without ReflectedType support, and without the information about the original type from xunit. :(
add @blairconrad

Collaborator

jeremymeng commented Jun 21, 2016

Not sure how to get ReflectedMethodData working on .NET Core, without ReflectedType support, and without the information about the original type from xunit. :(
add @blairconrad

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jun 21, 2016

Member

Ah, @jeremymeng, I didn't realise that ReflectedType is not supported on .NET Core. I don't suppose it's one of the pieces that's coming back in the near future?

Up 'til now (and maybe still) I'm sure that xUnit thought it was providing plenty information.

Unfortunately, I'd racked my brain for a while to come up with even the solution we have.
One option would just be to do away with the test hierarchy altogether, I suppose.

Hmm. Is all reflection off the table? Would it be possible to look in the current assembly for all types that extend ArgumentConstraintTestBase?

Member

blairconrad commented Jun 21, 2016

Ah, @jeremymeng, I didn't realise that ReflectedType is not supported on .NET Core. I don't suppose it's one of the pieces that's coming back in the near future?

Up 'til now (and maybe still) I'm sure that xUnit thought it was providing plenty information.

Unfortunately, I'd racked my brain for a while to come up with even the solution we have.
One option would just be to do away with the test hierarchy altogether, I suppose.

Hmm. Is all reflection off the table? Would it be possible to look in the current assembly for all types that extend ArgumentConstraintTestBase?

@jeremymeng

This comment has been minimized.

Show comment
Hide comment
@jeremymeng

jeremymeng Jun 21, 2016

Collaborator

I don't suppose it's one of the pieces that's coming back in the near future?

Not as far as I know.

Up 'til now (and maybe still) I'm sure that xUnit thought it was providing plenty information.

It doesn't hurt to ask for it. I will log an issue and see how it goes. Also it seems possible to create new xunit data discoverer and provide original type when retrieving data (https://github.com/xunit/xunit/blob/6381c93738860481af501a2372f796492b6e8d42/src/xunit.core/Sdk/DataDiscoverer.cs).

Collaborator

jeremymeng commented Jun 21, 2016

I don't suppose it's one of the pieces that's coming back in the near future?

Not as far as I know.

Up 'til now (and maybe still) I'm sure that xUnit thought it was providing plenty information.

It doesn't hurt to ask for it. I will log an issue and see how it goes. Also it seems possible to create new xunit data discoverer and provide original type when retrieving data (https://github.com/xunit/xunit/blob/6381c93738860481af501a2372f796492b6e8d42/src/xunit.core/Sdk/DataDiscoverer.cs).

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Jun 21, 2016

Member

@blairconrad there might be another way, which doesn't require a custom attribute:

  • In the base class:
    • Make all test methods virtual
  • In the derived classes:
    • Override the test methods, just calling the base implementation
    • Add the relevant MemberData attributes
Member

thomaslevesque commented Jun 21, 2016

@blairconrad there might be another way, which doesn't require a custom attribute:

  • In the base class:
    • Make all test methods virtual
  • In the derived classes:
    • Override the test methods, just calling the base implementation
    • Add the relevant MemberData attributes
@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Jun 21, 2016

Member

Yes, that works. It's sadly verbose, but always an option.
On 21 Jun 2016 10:05 p.m., "Thomas Levesque" notifications@github.com
wrote:

@blairconrad https://github.com/blairconrad there might be another way,
which doesn't require a custom attribute:

  • In the base class:
    • Make all test methods virtual
  • In the derived class
    • Override the test methods, just calling the base implementation
    • Add the relevant MemberData attributes


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#617 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AApXSE1S56HBVRGatvvrjsK4nq3p5Ksaks5qOFIzgaJpZM4HjNMh
.

Member

adamralph commented Jun 21, 2016

Yes, that works. It's sadly verbose, but always an option.
On 21 Jun 2016 10:05 p.m., "Thomas Levesque" notifications@github.com
wrote:

@blairconrad https://github.com/blairconrad there might be another way,
which doesn't require a custom attribute:

  • In the base class:
    • Make all test methods virtual
  • In the derived class
    • Override the test methods, just calling the base implementation
    • Add the relevant MemberData attributes


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#617 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AApXSE1S56HBVRGatvvrjsK4nq3p5Ksaks5qOFIzgaJpZM4HjNMh
.

@jeremymeng

This comment has been minimized.

Show comment
Hide comment
@jeremymeng

jeremymeng Jun 21, 2016

Collaborator

Also it seems possible to create new xunit data discoverer and provide original type when retrieving data

[answer to myself] Nope. xunit doesn't have the info for .NET Core either.

Looks like the verbose option is the way to go?

Collaborator

jeremymeng commented Jun 21, 2016

Also it seems possible to create new xunit data discoverer and provide original type when retrieving data

[answer to myself] Nope. xunit doesn't have the info for .NET Core either.

Looks like the verbose option is the way to go?

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Jun 21, 2016

Member

@jeremymeng, a bit off-topic, but how did you setup your build in AppVeyor? I tried several times to make FakeItEasy build on AppVeyor, but I always get a Ruby error:
https://ci.appveyor.com/project/thomaslevesque/fakeiteasy/build/57

Member

thomaslevesque commented Jun 21, 2016

@jeremymeng, a bit off-topic, but how did you setup your build in AppVeyor? I tried several times to make FakeItEasy build on AppVeyor, but I always get a Ruby error:
https://ci.appveyor.com/project/thomaslevesque/fakeiteasy/build/57

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Jun 21, 2016

Member

Looks like the verbose option is the way to go?

I think so. I sent a PR: #780

Member

thomaslevesque commented Jun 21, 2016

Looks like the verbose option is the way to go?

I think so. I sent a PR: #780

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jun 22, 2016

Member

Sorry I'm ignorant, but I was wondering if anyone knew the answer to

Is all reflection off the table? Would it be possible to look in the current assembly for all types that extend ArgumentConstraintTestBase?

Member

blairconrad commented Jun 22, 2016

Sorry I'm ignorant, but I was wondering if anyone knew the answer to

Is all reflection off the table? Would it be possible to look in the current assembly for all types that extend ArgumentConstraintTestBase?

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Jun 22, 2016

Member

@jeremymeng the reflective stuff is gone with the merge of #780

Member

adamralph commented Jun 22, 2016

@jeremymeng the reflective stuff is gone with the merge of #780

@jeremymeng

This comment has been minimized.

Show comment
Hide comment
@jeremymeng

jeremymeng Jun 22, 2016

Collaborator

a bit off-topic, but how did you setup your build in AppVeyor?

@thomaslevesque I only used two individual rake targets: bundle exec rake unit and bundle exec rake pack. I think the failure is due to older version of ruby. AppVeyor has ruby 1.9.3 as default. You can use the following before running bundle.

set PATH=C:\Ruby23-x64\bin;%PATH%
Collaborator

jeremymeng commented Jun 22, 2016

a bit off-topic, but how did you setup your build in AppVeyor?

@thomaslevesque I only used two individual rake targets: bundle exec rake unit and bundle exec rake pack. I think the failure is due to older version of ruby. AppVeyor has ruby 1.9.3 as default. You can use the following before running bundle.

set PATH=C:\Ruby23-x64\bin;%PATH%
@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Jun 22, 2016

Member

@thomaslevesque I only used two individual rake targets: bundle exec rake unit and bundle exec rake pack. I think the failure is due to older version of ruby. AppVeyor has ruby 1.9.3 as default. You can use the following before running bundle.

Thanks! I have other errors now, but at least the rake script runs ;)

Member

thomaslevesque commented Jun 22, 2016

@thomaslevesque I only used two individual rake targets: bundle exec rake unit and bundle exec rake pack. I think the failure is due to older version of ruby. AppVeyor has ruby 1.9.3 as default. You can use the following before running bundle.

Thanks! I have other errors now, but at least the rake script runs ;)

@jeremymeng

This comment has been minimized.

Show comment
Hide comment
@jeremymeng

jeremymeng Jun 22, 2016

Collaborator

Sorry I'm ignorant, but I was wondering if anyone knew the answer to

Is all reflection off the table? Would it be possible to look in the current assembly for all types that extend ArgumentConstraintTestBase?

@blairconrad I think it is possible. Not sure whether it is the answer that you are looking for: in RC2 lot of members are added back to System.Reflection namespace. However, ReflectedType is not one of them.

Collaborator

jeremymeng commented Jun 22, 2016

Sorry I'm ignorant, but I was wondering if anyone knew the answer to

Is all reflection off the table? Would it be possible to look in the current assembly for all types that extend ArgumentConstraintTestBase?

@blairconrad I think it is possible. Not sure whether it is the answer that you are looking for: in RC2 lot of members are added back to System.Reflection namespace. However, ReflectedType is not one of them.

src/FakeItEasy.dotnet/project.json
},
"frameworks": {
- "netstandard1.5": {
+ "netstandard1.6": {
"imports": [ "dotnet5.4" ]

This comment has been minimized.

@thomaslevesque

thomaslevesque Jun 28, 2016

Member

Is the imports still necessary? None of the dependencies seem to target dotnet5.4 (I didn't check them all, but Newtonsoft.Json targets netstandard1.0, and the System.* and Microsoft.* packages I checked target netstandard 1.6)

(of course, it's also possible that I completely misunderstand what imports does 😉)

@thomaslevesque

thomaslevesque Jun 28, 2016

Member

Is the imports still necessary? None of the dependencies seem to target dotnet5.4 (I didn't check them all, but Newtonsoft.Json targets netstandard1.0, and the System.* and Microsoft.* packages I checked target netstandard 1.6)

(of course, it's also possible that I completely misunderstand what imports does 😉)

This comment has been minimized.

@jeremymeng

jeremymeng Jun 28, 2016

Collaborator

Yes, until Castle.Core publish a package with netstandard support.

@jeremymeng

jeremymeng Jun 28, 2016

Collaborator

Yes, until Castle.Core publish a package with netstandard support.

This comment has been minimized.

@thomaslevesque

thomaslevesque Jun 28, 2016

Member

Ah, yes... I only checked the dependencies that had changed, so I missed it.

@thomaslevesque

thomaslevesque Jun 28, 2016

Member

Ah, yes... I only checked the dependencies that had changed, so I missed it.

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Jun 28, 2016

Member

Thanks for keeping this up to date @jeremymeng!

I see only one item remaining in your checklist:

usage of custom AssemblyLoadContext needs review.

Unfortunately, I don't think any of us @FakeItEasy/owners know anything about that... Is there someone else who could help here?

Member

thomaslevesque commented Jun 28, 2016

Thanks for keeping this up to date @jeremymeng!

I see only one item remaining in your checklist:

usage of custom AssemblyLoadContext needs review.

Unfortunately, I don't think any of us @FakeItEasy/owners know anything about that... Is there someone else who could help here?

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Jul 1, 2016

Member

@jeremymeng there's no need to bump the version in a feature branch, we'll take care of that independently on master. Bumping it on a feature branch will only make the merge more difficult, and who knows, we might release 2.3, 2.4, 2.5 in the meantime...

Member

adamralph commented Jul 1, 2016

@jeremymeng there's no need to bump the version in a feature branch, we'll take care of that independently on master. Bumping it on a feature branch will only make the merge more difficult, and who knows, we might release 2.3, 2.4, 2.5 in the meantime...

@jeremymeng

This comment has been minimized.

Show comment
Hide comment
@jeremymeng

jeremymeng Jul 1, 2016

Collaborator

Thanks @adamralph I revert it.

Collaborator

jeremymeng commented Jul 1, 2016

Thanks @adamralph I revert it.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jul 19, 2016

Member

I'd considered just using GetTypeInformation throughout, but found a couple of failing unit tests in the .dotnet project. I forget which ones. 😕

Member

blairconrad commented Jul 19, 2016

I'd considered just using GetTypeInformation throughout, but found a couple of failing unit tests in the .dotnet project. I forget which ones. 😕

@@ -101,7 +101,9 @@ public override IFakeOptionsForWrappers<T> Wrapping(T wrappedInstance)
Guard.AgainstNull(wrappedInstance, nameof(wrappedInstance));
var wrapper = new FakeWrapperConfigurator<T>(this, wrappedInstance);
+#if FEATURE_SELF_INITIALIZED_FAKES
this.ConfigureFake(fake => wrapper.ConfigureFakeToWrap(fake));

This comment has been minimized.

@thomaslevesque

thomaslevesque Jul 19, 2016

Member

I think this statement is still necessary even without SelfInitializedFakes; it's what configures the fake to forward calls to the wrapped object. Inside ConfigureFakeToWrap, the call to AddRecordingRuleWhenRecorderIsSpecified should be disabled, though.

@thomaslevesque

thomaslevesque Jul 19, 2016

Member

I think this statement is still necessary even without SelfInitializedFakes; it's what configures the fake to forward calls to the wrapped object. Inside ConfigureFakeToWrap, the call to AddRecordingRuleWhenRecorderIsSpecified should be disabled, though.

This comment has been minimized.

@thomaslevesque

thomaslevesque Jul 19, 2016

Member

Ah, I see that @blairconrad already fixed it

@thomaslevesque

thomaslevesque Jul 19, 2016

Member

Ah, I see that @blairconrad already fixed it

@@ -23,15 +21,14 @@ public class CallData
public CallData(MethodInfo method, IEnumerable<object> outputArguments, object returnValue)
{
this.Method = method;
- this.OutputArguments = outputArguments == null ? null : outputArguments.ToArray();
+ this.OutputArguments = outputArguments.ToArray();

This comment has been minimized.

@thomaslevesque

thomaslevesque Jul 19, 2016

Member

Did you mean to write outputArguments?.ToArray()? I assume that if there was a null check, it's because outputArguments can be null...

@thomaslevesque

thomaslevesque Jul 19, 2016

Member

Did you mean to write outputArguments?.ToArray()? I assume that if there was a null check, it's because outputArguments can be null...

This comment has been minimized.

@jeremymeng

jeremymeng Jul 19, 2016

Collaborator

No, I simply revert the file to the version prior to my serialization changes.

@jeremymeng

jeremymeng Jul 19, 2016

Collaborator

No, I simply revert the file to the version prior to my serialization changes.

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Jul 19, 2016

Member

@blairconrad @thomaslevesque I pushed a change to revert the json serialization change and exclude self initialized fakes for .NET Core. I disabled minimal code to made the .NET Core project to build so there might be things could be further excluded. Please have a look when you get a chance.

I had a few concerns, but it seems they've been addressed already; LGTM 👍

Thanks!

Member

thomaslevesque commented Jul 19, 2016

@blairconrad @thomaslevesque I pushed a change to revert the json serialization change and exclude self initialized fakes for .NET Core. I disabled minimal code to made the .NET Core project to build so there might be things could be further excluded. Please have a look when you get a chance.

I had a few concerns, but it seems they've been addressed already; LGTM 👍

Thanks!

@jeremymeng

This comment has been minimized.

Show comment
Hide comment
@jeremymeng

jeremymeng Jul 19, 2016

Collaborator

I'd considered just using GetTypeInformation throughout,

I didn't get any build errors so I am wondering what difference could GetTypeInformation bring? On .NET Core it just passes the call through.

I am seeing two test failures like below after setting outputName and remove the InternalsVisible line.

      System.InvalidCastException : Unable to cast object of type 'Castle.Proxies.TypeProxy' to type 'System.Reflection.IReflectableType'.
      Stack Trace:
           at System.Reflection.IntrospectionExtensions.GetTypeInfo(Type type)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Creation\DummyValueCreationSession.cs(149,0): at FakeItEasy.Creation.DummyValueCreationSession.ResolveByCreatingTaskStrategy.TryCreateDummyValue(Type typeOfDummy, Object& result)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Creation\DummyValueCreationSession.cs(83,0): at FakeItEasy.Creation.DummyValueCreationSession.TryResolveDummyValueWithAllAvailableStrategies(Type typeOfDummy, Object& result)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Creation\DummyValueCreationSession.cs(46,0): at FakeItEasy.Creation.DummyValueCreationSession.TryResolveDummyValue(Type typeOfDummy, Object& result)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Creation\DefaultFakeAndDummyManager.cs(48,0): at FakeItEasy.Creation.DefaultFakeAndDummyManager.TryCreateDummy(Type typeOfDummy, Object& result)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Core\DefaultReturnValueRule.cs(19,0): at FakeItEasy.Core.DefaultReturnValueRule.ResolveReturnValue(IInterceptedFakeObjectCall fakeObjectCall)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Core\DefaultReturnValueRule.cs(36,0): at FakeItEasy.Core.DefaultReturnValueRule.Apply(IInterceptedFakeObjectCall fakeObjectCall)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Core\FakeManager.cs(188,0): at FakeItEasy.Core.FakeManager.ApplyRule(CallRuleMetadata rule, IInterceptedFakeObjectCall fakeObjectCall)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Core\FakeManager.cs(144,0): at FakeItEasy.Core.FakeManager.FakeItEasy.Core.IFakeCallProcessor.Process(IInterceptedFakeObjectCall fakeObjectCall)
        C:\github\jeremymeng\FakeItEasy\tests\FakeItEasy.Tests\Core\FakeManagerTests.cs(506,0): at FakeItEasy.Tests.Core.FakeManagerTests.ProcessFakeObjectCall(IFakeCallProcessor fakeCallProcessor, IInterceptedFakeObjectCall interceptedCall)
        C:\github\jeremymeng\FakeItEasy\tests\FakeItEasy.Tests\Core\FakeManagerTests.cs(425,0): at FakeItEasy.Tests.Core.FakeManagerTests.Should_invoke_listener_when_call_is_intercepted()

I am surprised and puzzled that setting outputName would cause failures. Haven't got to the bottom of the failures, but another option is to move the .NET Core test project into a FakeItEasy.Tests folder (jeremymeng#2). Not any prettier than having the extra FakeItEasy.Tests.dotnet.dll...

Collaborator

jeremymeng commented Jul 19, 2016

I'd considered just using GetTypeInformation throughout,

I didn't get any build errors so I am wondering what difference could GetTypeInformation bring? On .NET Core it just passes the call through.

I am seeing two test failures like below after setting outputName and remove the InternalsVisible line.

      System.InvalidCastException : Unable to cast object of type 'Castle.Proxies.TypeProxy' to type 'System.Reflection.IReflectableType'.
      Stack Trace:
           at System.Reflection.IntrospectionExtensions.GetTypeInfo(Type type)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Creation\DummyValueCreationSession.cs(149,0): at FakeItEasy.Creation.DummyValueCreationSession.ResolveByCreatingTaskStrategy.TryCreateDummyValue(Type typeOfDummy, Object& result)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Creation\DummyValueCreationSession.cs(83,0): at FakeItEasy.Creation.DummyValueCreationSession.TryResolveDummyValueWithAllAvailableStrategies(Type typeOfDummy, Object& result)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Creation\DummyValueCreationSession.cs(46,0): at FakeItEasy.Creation.DummyValueCreationSession.TryResolveDummyValue(Type typeOfDummy, Object& result)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Creation\DefaultFakeAndDummyManager.cs(48,0): at FakeItEasy.Creation.DefaultFakeAndDummyManager.TryCreateDummy(Type typeOfDummy, Object& result)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Core\DefaultReturnValueRule.cs(19,0): at FakeItEasy.Core.DefaultReturnValueRule.ResolveReturnValue(IInterceptedFakeObjectCall fakeObjectCall)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Core\DefaultReturnValueRule.cs(36,0): at FakeItEasy.Core.DefaultReturnValueRule.Apply(IInterceptedFakeObjectCall fakeObjectCall)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Core\FakeManager.cs(188,0): at FakeItEasy.Core.FakeManager.ApplyRule(CallRuleMetadata rule, IInterceptedFakeObjectCall fakeObjectCall)
        C:\github\jeremymeng\FakeItEasy\src\FakeItEasy\Core\FakeManager.cs(144,0): at FakeItEasy.Core.FakeManager.FakeItEasy.Core.IFakeCallProcessor.Process(IInterceptedFakeObjectCall fakeObjectCall)
        C:\github\jeremymeng\FakeItEasy\tests\FakeItEasy.Tests\Core\FakeManagerTests.cs(506,0): at FakeItEasy.Tests.Core.FakeManagerTests.ProcessFakeObjectCall(IFakeCallProcessor fakeCallProcessor, IInterceptedFakeObjectCall interceptedCall)
        C:\github\jeremymeng\FakeItEasy\tests\FakeItEasy.Tests\Core\FakeManagerTests.cs(425,0): at FakeItEasy.Tests.Core.FakeManagerTests.Should_invoke_listener_when_call_is_intercepted()

I am surprised and puzzled that setting outputName would cause failures. Haven't got to the bottom of the failures, but another option is to move the .NET Core test project into a FakeItEasy.Tests folder (jeremymeng#2). Not any prettier than having the extra FakeItEasy.Tests.dotnet.dll...

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jul 19, 2016

Member

Those are exactly the errors I'm seeing. I had thought that maybe before the change we were accessing the Castle.Core TypeInfo, but now aren't, and that was the difference, but I didn't have time to investigate.

Member

blairconrad commented Jul 19, 2016

Those are exactly the errors I'm seeing. I had thought that maybe before the change we were accessing the Castle.Core TypeInfo, but now aren't, and that was the difference, but I didn't have time to investigate.

@jeremymeng

This comment has been minimized.

Show comment
Hide comment
@jeremymeng

jeremymeng Jul 19, 2016

Collaborator

For the renamed output I can see that the created fake object has some invalid (empty) types, but not sure why yet

+       DeclaringType   {}  System.Type {Castle.Proxies.TypeProxy}
+       ReflectedType   {}  System.Type {Castle.Proxies.TypeProxy}
+       ReturnParameter {}  System.Reflection.ParameterInfo {Castle.Proxies.ParameterInfoProxy}
+       ReturnType  {}  System.Type {Castle.Proxies.TypeProxy}

In a passing run, these have valid types like {System.Void}, etc., and are of type System.RuntimeType.

Collaborator

jeremymeng commented Jul 19, 2016

For the renamed output I can see that the created fake object has some invalid (empty) types, but not sure why yet

+       DeclaringType   {}  System.Type {Castle.Proxies.TypeProxy}
+       ReflectedType   {}  System.Type {Castle.Proxies.TypeProxy}
+       ReturnParameter {}  System.Reflection.ParameterInfo {Castle.Proxies.ParameterInfoProxy}
+       ReturnType  {}  System.Type {Castle.Proxies.TypeProxy}

In a passing run, these have valid types like {System.Void}, etc., and are of type System.RuntimeType.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jul 20, 2016

Member

@jeremymeng, were you able to debug into the tests? For the life of me, I could not. I even resorted to installing the new TestDriven.NET and explicitly loading FakeManagerTests.cs to the FakeItEasy.Tests.dotnet project, but no luck.

Member

blairconrad commented Jul 20, 2016

@jeremymeng, were you able to debug into the tests? For the life of me, I could not. I even resorted to installing the new TestDriven.NET and explicitly loading FakeManagerTests.cs to the FakeItEasy.Tests.dotnet project, but no luck.

@jeremymeng

This comment has been minimized.

Show comment
Hide comment
@jeremymeng

jeremymeng Jul 20, 2016

Collaborator

@blairconrad The debugging experiences hasn't been smooth for me either. My VS doesn't show any tests after I set the outputName build option. I logged an issue for it. Anyway I found that the following worked for me

  • close and reopen the solution
  • unload all the other test projects so only .NET Core tests showing up in the VS Test Explorer
  • replace Castle.Core.dll under %USERPROFILE%.nuget\packages with a debug version that I built so that I can step into the Castle.Core source.

Still haven't figured out why the differences.

Collaborator

jeremymeng commented Jul 20, 2016

@blairconrad The debugging experiences hasn't been smooth for me either. My VS doesn't show any tests after I set the outputName build option. I logged an issue for it. Anyway I found that the following worked for me

  • close and reopen the solution
  • unload all the other test projects so only .NET Core tests showing up in the VS Test Explorer
  • replace Castle.Core.dll under %USERPROFILE%.nuget\packages with a debug version that I built so that I can step into the Castle.Core source.

Still haven't figured out why the differences.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jul 20, 2016

Member

Thanks, @jeremymeng, I will give that a go later. (I had just been hoping to debug into FIE. Getting into Castle.Core would be a bonus!)

@adamralph, @thomaslevesque, the difficulty with the rename (and ugliness of the extra dir) make me wonder if we aren't better off just keeping the InternalsVisibleTo for FakeItEasy.Tests.dotnet and updating the approved API txt. Thoughts?

Member

blairconrad commented Jul 20, 2016

Thanks, @jeremymeng, I will give that a go later. (I had just been hoping to debug into FIE. Getting into Castle.Core would be a bonus!)

@adamralph, @thomaslevesque, the difficulty with the rename (and ugliness of the extra dir) make me wonder if we aren't better off just keeping the InternalsVisibleTo for FakeItEasy.Tests.dotnet and updating the approved API txt. Thoughts?

Change the name of the .NET Core test project (#3)
Use the same name as the net45 test project thus avoid extra InternalsVisibleTo attribute.
@jeremymeng

This comment has been minimized.

Show comment
Hide comment
@jeremymeng

jeremymeng Jul 20, 2016

Collaborator

@blairconrad changing "name" instead of setting "outputName" seems working.

Collaborator

jeremymeng commented Jul 20, 2016

@blairconrad changing "name" instead of setting "outputName" seems working.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jul 20, 2016

Member

I see your new PR. That is very interesting. How did you find it?

Member

blairconrad commented Jul 20, 2016

I see your new PR. That is very interesting. How did you find it?

@blairconrad

This comment has been minimized.

Show comment
Hide comment
Member

blairconrad commented Jul 20, 2016

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jul 20, 2016

Member

Nice, job @jeremymeng! Passing rake approve. 🎉

We have only two tasks open.
I have almost no knowledge of the AssemblyLoadContext usage, but will certainly read it over again. As well as examining the whole PR once again. @thomaslevesque and @adamralph, unless there's another barrier, I think we're ready for a final review!

Member

blairconrad commented Jul 20, 2016

Nice, job @jeremymeng! Passing rake approve. 🎉

We have only two tasks open.
I have almost no knowledge of the AssemblyLoadContext usage, but will certainly read it over again. As well as examining the whole PR once again. @thomaslevesque and @adamralph, unless there's another barrier, I think we're ready for a final review!

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Jul 20, 2016

Member

Great stuff! I'll get to it ASAP.

Member

adamralph commented Jul 20, 2016

Great stuff! I'll get to it ASAP.

using FakeItEasy.Core;
using FakeItEasy.Creation;
+#if FEATURE_BINARY_SERIALIZATION

This comment has been minimized.

@blairconrad

blairconrad Jul 20, 2016

Member

should be #if FEATURE_SELF_INITIALIZED_FAKES ?

@blairconrad

blairconrad Jul 20, 2016

Member

should be #if FEATURE_SELF_INITIALIZED_FAKES ?

This comment has been minimized.

@jeremymeng

jeremymeng Jul 20, 2016

Collaborator

Did you copy-n-pasted a wrong one? I submitted jeremymeng#4

@jeremymeng

jeremymeng Jul 20, 2016

Collaborator

Did you copy-n-pasted a wrong one? I submitted jeremymeng#4

This comment has been minimized.

@blairconrad

blairconrad Jul 20, 2016

Member

Yes, I seem to have! Odd, since you'd think I would've just grabbed the line I removed from the top of the file.
Oh. Now that I think of it, I seem to remember typing the #if FEATURE_ and relying on Intellisense. Or the way I use it, apparently Unintellisense. 😠

@blairconrad

blairconrad Jul 20, 2016

Member

Yes, I seem to have! Odd, since you'd think I would've just grabbed the line I removed from the top of the file.
Oh. Now that I think of it, I seem to remember typing the #if FEATURE_ and relying on Intellisense. Or the way I use it, apparently Unintellisense. 😠

src/FakeItEasy.dotnet/FakeItEasy.nuspec
+ <file src="..\FakeItEasy\bin\Release\FakeItEasy.xml" target="lib\net40" />
+ <file src="bin\Release\FakeItEasy.dll" target="lib\netstandard1.5" />
+ <file src="bin\Release\FakeItEasy.pdb" target="lib\netstandard1.5" />
+ <file src="bin\Release\FakeItEasy.xml" target="lib\netstandard1.5" />

This comment has been minimized.

@blairconrad

blairconrad Jul 20, 2016

Member

I probably should've asked about this earlier, but if our library targets netstandard1.5, why do we have dependencies for .NETStandard1.6? I am still struggling with this, and probably way off-base, but I would have thought that if we depended on .NETStandard1.6, we'd need to make a .NETStandard1.6 library. Or if we were producing 1.5, we could get away with 1.5 dependencies.

@blairconrad

blairconrad Jul 20, 2016

Member

I probably should've asked about this earlier, but if our library targets netstandard1.5, why do we have dependencies for .NETStandard1.6? I am still struggling with this, and probably way off-base, but I would have thought that if we depended on .NETStandard1.6, we'd need to make a .NETStandard1.6 library. Or if we were producing 1.5, we could get away with 1.5 dependencies.

This comment has been minimized.

@jeremymeng

jeremymeng Jul 21, 2016

Collaborator

Ah I think we need to target 1.6 because of the Microsoft.Extensions.DependencyModel dependency. I forgot to update the nuspec file.

@jeremymeng

jeremymeng Jul 21, 2016

Collaborator

Ah I think we need to target 1.6 because of the Microsoft.Extensions.DependencyModel dependency. I forgot to update the nuspec file.

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Jul 21, 2016

Member

I have almost no knowledge of the AssemblyLoadContext usage

Neither do I... The API documentation is completely useless, without even a basic description. There doesn't seem to be much literature about it; I found this, but I don't know if it's still relevant (things might have changed in RTM)

Member

thomaslevesque commented Jul 21, 2016

I have almost no knowledge of the AssemblyLoadContext usage

Neither do I... The API documentation is completely useless, without even a basic description. There doesn't seem to be much literature about it; I found this, but I don't know if it's still relevant (things might have changed in RTM)

+ var fakeItEasyLibraryName = TypeCatalogue.FakeItEasyAssembly.GetName().Name;
+ var context = Microsoft.Extensions.DependencyModel.DependencyContext.Default;
+
+ var runtimeLib = context.RuntimeLibraries

This comment has been minimized.

@blairconrad

blairconrad Jul 21, 2016

Member

Why RuntimeLibraries instead of CompileTime? Sorry, I know nothing. But I'd expect that if an assembly references FakeItEasy it would do so at compile time. (Perhaps this is a silly question, but docs are hard to come by - I couldn't learn anything about DepenencyContext.)

@blairconrad

blairconrad Jul 21, 2016

Member

Why RuntimeLibraries instead of CompileTime? Sorry, I know nothing. But I'd expect that if an assembly references FakeItEasy it would do so at compile time. (Perhaps this is a silly question, but docs are hard to come by - I couldn't learn anything about DepenencyContext.)

blairconrad and others added some commits Jul 21, 2016

Stop adding AttributesToAvoidReplicating (#5)
The SecurityPermissionAttribute has been
automatically excluded from replication since
Caslte.Core 2.5.2.
+ var context = Microsoft.Extensions.DependencyModel.DependencyContext.Default;
+
+ var runtimeLib = context.RuntimeLibraries
+ .SingleOrDefault(library => string.Equals(library.Name, assembly.Name(), System.StringComparison.Ordinal));

This comment has been minimized.

@blairconrad

blairconrad Jul 24, 2016

Member

@jeremymeng, do you know why it's okay to equate a library name with an assembly name?
It's not that I believe it will stop working (it's probably the case that there will be name agreement in all but pathological cases), but I'm just wondering about the justification, aside from perhaps observation that it works.
The lack of documentation for these things is killing me.

@blairconrad

blairconrad Jul 24, 2016

Member

@jeremymeng, do you know why it's okay to equate a library name with an assembly name?
It's not that I believe it will stop working (it's probably the case that there will be name agreement in all but pathological cases), but I'm just wondering about the justification, aside from perhaps observation that it works.
The lack of documentation for these things is killing me.

This comment has been minimized.

@blairconrad

blairconrad Jul 24, 2016

Member

I notice that a library has a list of assemblies in it. Should we be looking at those and comparing names instead of the library name?

@blairconrad

blairconrad Jul 24, 2016

Member

I notice that a library has a list of assemblies in it. Should we be looking at those and comparing names instead of the library name?

This comment has been minimized.

@jeremymeng

jeremymeng Jul 25, 2016

Collaborator

do you know why it's okay to equate a library name with an assembly name?

I couldn't find any docs either. I might have modeled my code after some usage in ASP.NET repro, so there could be things that don't apply. I will try to track down some docs/specs.

I notice that a library has a list of assemblies in it. Should we be looking at those and comparing names instead of the library name?

Under debugger, they are all zero-length. I don't know what they are for yet.

@jeremymeng

jeremymeng Jul 25, 2016

Collaborator

do you know why it's okay to equate a library name with an assembly name?

I couldn't find any docs either. I might have modeled my code after some usage in ASP.NET repro, so there could be things that don't apply. I will try to track down some docs/specs.

I notice that a library has a list of assemblies in it. Should we be looking at those and comparing names instead of the library name?

Under debugger, they are all zero-length. I don't know what they are for yet.

This comment has been minimized.

@blairconrad

blairconrad Jul 25, 2016

Member

Thanks, @jeremymeng. Don't take too much time on looking for docs (unless you can get them published! :-D). If the values you're using look good, that's enough for me for now. We'll hook up the tests and so long as things keep working, we'll muddle along.

@blairconrad

blairconrad Jul 25, 2016

Member

Thanks, @jeremymeng. Don't take too much time on looking for docs (unless you can get them published! :-D). If the values you're using look good, that's enough for me for now. We'll hook up the tests and so long as things keep working, we'll muddle along.

This comment has been minimized.

@jeremymeng

jeremymeng Jul 26, 2016

Collaborator

I think DependencyContext provides compilation information at runtime. If you look under the dotnet build output folder you should see an .deps.json. As far as I can see under debugger, context contains information from that file. CompileLibraries include all the non-meta libraries in targets section, while RuntimeLibraries seem to include all the runtime dependencies.

@jeremymeng

jeremymeng Jul 26, 2016

Collaborator

I think DependencyContext provides compilation information at runtime. If you look under the dotnet build output folder you should see an .deps.json. As far as I can see under debugger, context contains information from that file. CompileLibraries include all the non-meta libraries in targets section, while RuntimeLibraries seem to include all the runtime dependencies.

+ var appDomainAssembliesReferencingFakeItEasy = candidateLibraries
+ .SelectMany(library => library.GetDefaultAssemblyNames(context))
+ .Select(Assembly.Load)
+ .Where(a => !a.IsDynamic);

This comment has been minimized.

@blairconrad

blairconrad Jul 24, 2016

Member

If we just loaded the assembly, is there any way for it to be dynamic?

@blairconrad

blairconrad Jul 24, 2016

Member

If we just loaded the assembly, is there any way for it to be dynamic?

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jul 25, 2016

Member

@adamralph, @thomaslevesque: @jeremymeng has merged some minor cleanup (mostly grammar and variable names) around the extension point discovery classes. With this change, I would be content to merge the PR to master.

As discussed earlier, there's still more work to be done in order to release a .NET Core-supporting FakeItEasy, but my personal opinion is that this PR is in good enough shape that a merge would allow us to continue working in master, via our usual method of smaller PRs.

Take a gander when you've time, please, and if there's anything you'd like changed and have time to explain but not to do, I'll give 'er a whirl.

(Oh, you've probably noticed some outstanding questions about assembly/library handling, but I'm content to make whatever changes need to be made for those (if any) in master. They are only for the .NET Core branch, and I'd prefer to get the .NET Core integration tests hooked up before we mucked about too much in that area anyhow.)

Member

blairconrad commented Jul 25, 2016

@adamralph, @thomaslevesque: @jeremymeng has merged some minor cleanup (mostly grammar and variable names) around the extension point discovery classes. With this change, I would be content to merge the PR to master.

As discussed earlier, there's still more work to be done in order to release a .NET Core-supporting FakeItEasy, but my personal opinion is that this PR is in good enough shape that a merge would allow us to continue working in master, via our usual method of smaller PRs.

Take a gander when you've time, please, and if there's anything you'd like changed and have time to explain but not to do, I'll give 'er a whirl.

(Oh, you've probably noticed some outstanding questions about assembly/library handling, but I'm content to make whatever changes need to be made for those (if any) in master. They are only for the .NET Core branch, and I'd prefer to get the .NET Core integration tests hooked up before we mucked about too much in that area anyhow.)

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Jul 25, 2016

Member

👍 great work!

I'm away for the whole of August and this week will be busy prepping so I probably won't have much time to have a look. In general, I'm happy with the merge if it no way affects the nupkg which master is producing, i.e. no added TFN (just the current net40 DLL) and no regressions within that DLL.

Member

adamralph commented Jul 25, 2016

👍 great work!

I'm away for the whole of August and this week will be busy prepping so I probably won't have much time to have a look. In general, I'm happy with the merge if it no way affects the nupkg which master is producing, i.e. no added TFN (just the current net40 DLL) and no regressions within that DLL.

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Jul 25, 2016

Member

In general, I'm happy with the merge if it no way affects the nupkg which master is producing, i.e. no added TFN (just the current net40 DLL) and no regressions within that DLL.

Agreed. If if doesn't affect the net40 version, let's merge it now. It will be easier to fix the remaining issues in smaller PRs.

Member

thomaslevesque commented Jul 25, 2016

In general, I'm happy with the merge if it no way affects the nupkg which master is producing, i.e. no added TFN (just the current net40 DLL) and no regressions within that DLL.

Agreed. If if doesn't affect the net40 version, let's merge it now. It will be easier to fix the remaining issues in smaller PRs.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jul 29, 2016

Member

Wow! I somehow missed both of these comments from 4 days ago.
Sorry about that.

I've rereviewed. The approved API is untouched. If someone would merge jeremymeng/FakeItEasy#8, we'll have exactly the same tests run in both master and @jeremymeng's coreclr.

Our main package .nuspec has changed in that there is a new section:

<dependencies>
  <group targetFramework=".NETFramework4.0" />
</dependencies>

which I do not think is harmful. But we could remove.
If nobody minds this textual change to the nuspec, I (or someone else) can squashmerge after the integration tests change.

Member

blairconrad commented Jul 29, 2016

Wow! I somehow missed both of these comments from 4 days ago.
Sorry about that.

I've rereviewed. The approved API is untouched. If someone would merge jeremymeng/FakeItEasy#8, we'll have exactly the same tests run in both master and @jeremymeng's coreclr.

Our main package .nuspec has changed in that there is a new section:

<dependencies>
  <group targetFramework=".NETFramework4.0" />
</dependencies>

which I do not think is harmful. But we could remove.
If nobody minds this textual change to the nuspec, I (or someone else) can squashmerge after the integration tests change.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jul 29, 2016

Member

@jeremymeng has graciously merged jeremymeng/FakeItEasy#8.
Assuming nobody else has done so by (my) tomorrow morning, I'll merge this PR as a single commit then.

Member

blairconrad commented Jul 29, 2016

@jeremymeng has graciously merged jeremymeng/FakeItEasy#8.
Assuming nobody else has done so by (my) tomorrow morning, I'll merge this PR as a single commit then.

@blairconrad blairconrad merged commit 46becf4 into FakeItEasy:master Jul 30, 2016

2 checks passed

continuous-integration/codebetter-ci Finished TeamCity Build FakeItEasy :: FakeItEasy : Tests passed: 1287
Details
continuous-integration/codebetter-ci/approval Finished TeamCity Build FakeItEasy :: FakeItEasy Approval : Tests passed: 1
Details
@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jul 30, 2016

Member

@jeremymeng, fantastic work.
Thanks so much for your efforts, and if you're not sick of us yet, I look forward to other opportunities to collaborate.

Member

blairconrad commented Jul 30, 2016

@jeremymeng, fantastic work.
Thanks so much for your efforts, and if you're not sick of us yet, I look forward to other opportunities to collaborate.

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Jul 30, 2016

Member

@jeremymeng, fantastic work.
Thanks so much for your efforts, and if you're not sick of us yes, I look forward to other opportunities to collaborate.

Great work indeed, thanks @jeremymeng !

Member

thomaslevesque commented Jul 30, 2016

@jeremymeng, fantastic work.
Thanks so much for your efforts, and if you're not sick of us yes, I look forward to other opportunities to collaborate.

Great work indeed, thanks @jeremymeng !

blairconrad added a commit to blairconrad/FakeItEasy that referenced this pull request Jul 30, 2016

[WIP] Porting to .Net Core (#617)
Supports building a .NET Core library dependant on .NET Standard packages.
The .NET Core library is not (yet) built by default nor included in the main NuGet package.
Unit tests have been converted, leaving integration, spec, and approval tests to go.
Further .NET Core work will be done in smaller commits merged to master, ultimately culminating in the flipping of the "feature toggle", when the NuGet package will include that library as well as the .NET 4.0 version.

@blairconrad blairconrad deleted the jeremymeng:coreclr branch Jul 30, 2016

@adamralph adamralph changed the title from [WIP] Porting to .Net Core to Porting to .Net Core Aug 3, 2016

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