-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use EnterContextualReflection to address .NET Core dependency loading issues #942
Conversation
CI is very confusing. Looks like Travis is failing due to the changes in #835 - and for some reason has just started running again, after not running for the last 7 months? No idea what's caused that. I think the issue is just that net50 and netcoreapp21 aren't installed. I think this is probably our cue to remove Travis, given we're now maintaining 3 separate CI systems. As far as I'm aware, everything tested on Travis is also covered by Azure. |
@ChrisMaddock How come this PR is including my changes on the other PR, especially since you've asked me to make changes before committing? |
OK, never mind. I see you have a comment about that in the issue. |
Hello @ChrisMaddock, thank you for the fix.
One last thing, I've noticed that it claims the environment is .NET Framework instead of .NET Core 3.1 |
Thanks for trying it out @bouchraRekhadda! Just to check, that's definitely with the NUnit.ConsoleRunner.NetCore package, and not the normal one? Could you come up with a minimal example that reproduces this, please? Unfortunately there's not enough info in the stack trace to work out what's going wrong. |
Hello @ChrisMaddock,
I'll try to provide tomorrow a reproduction code for binary serialization particular case. Regarding other use cases with assembly load issues, we have one more where we use code compilation at runtime. Please find more details in nunit-sample repository with a little description of how to reproduce in the README (I hope this will be helpful). |
I haven't managed to catch up with all issues that is fixed in this version of the console runner, so could be I have tested things that is known not to work But I downloaded it and gave it a run. Had to add Also still cannot run tests out-of-process. (Since we build all our test-assemblies to separate outputs, some shared dlls need to have some static data initialized, and the runtime gets confused about this; first test-assembly works, the seconds don't initialize the static, but don't find the data either) Also, for Bamboo reasons, we need to have NUnit2 output. Edit: We are target .Net Core 2.2 |
Hello @ChrisMaddock Regarding the above stack-trace, and after pulling my hair out, I finally understood why it occurred: we have disabled the Target Framework attribute generation through So after enabling the attribute generation, I can finally confirm that the current version 3.13.0 fixes the serialization issue we had (cf. #939 ) 🎉. |
Wow! Just read this and realized we have another way we should test the engine... disabling generation of TargetFrameworkAttribute! Thanks for that @bouchraRekhadda. Yes, The engine currently assumes the current framework. But the change you are testing was based on an earlier version of my unmerged PR #937 which changes that to assume .NET Framework, since I've only ever seen a missing attribute for netfx. Really, we shouldn't assume anything but when the attribute is not there, we should go further in trying to figure out what the assembly actually targets. 😉 |
Regarding this scenario (code emission at runtime), unfortunately I still reproduce the failures with 3.13.0. @ChrisMaddock, @CharliePoole what do you advise for this case ? |
@bouchraRekhadda Great news we've now got the normal case up and running! 🎉 Your compilation at run-time case, afraid I have no idea how to get that working. 😅 I would have thought @runehalfdan Hi - good to hear from you again! 👋 Please could you elaborate on "Had to add true to all test-projects"? It would be really helpful if you could add a small repo for this problem (in a new issue, perhaps!). As things stand, we don't currently have any more reproducible examples which are not working, other than @bouchraRekhadda's code-compliation-at-runtime, which I expect is a very different problem! On the other points you mention:
@CharliePoole - What are the chances that the first user to try this would encounter the TargetFrameworkAttribute issue we'd just been talking about, eh? 😄 |
I'm going to merge this, as it seems to be at least one step closer to where we want to be. Thanks to those who've been able to try it out. 🙌 |
Edited original;markdown problems. Was CopyLocalLockFileAssemblies that must be true. |
Hello @ChrisMaddock, first of all thank you for the fix, I can't wait to update our Test Runner with this new version. Regarding this:
Always using version 3.13.0 provided through this pull request, we have tried enforcing the current reflection context by simply doing |
Hello, @ChrisMaddock, @CharliePoole, Thanks |
Hi @bouchraRekhadda - probably not imminently I'm afraid, there's a few more issue we need to look into first like #956 and #843. You can get pre-release builds from our MyGet feed though, that should cover what you need? 🙂 https://www.myget.org/feed/nunit/package/nuget/NUnit.ConsoleRunner.NetCore |
We were hoping to get a beta release like To be more precise, we only need NUnit.Engine including the fix from this pull request; we could re-package internally the |
Hello, Thanks |
Hopefully fixes #828 and #916
This uses the new EnterContextualReflection API introduced in netcore30, which will hopefully solve our .NET Core dependency loading issues. I wasn't aware of this previously as I don't think Microsoft's docs have yet been updated to include this - however there's some design documentation at the link below. (Thanks @Learfin for mentioning it in your write-up!) From what I understand, the API was added to tackle issues with xUnit that seem to be very similar to the problems we've been hitting.
https://github.com/dotnet/coreclr/blob/master/Documentation/design-docs/AssemblyLoadContext.ContextualReflection.md
I wanted to add a test for this, but unfortunately it's not really possible our current build structure. It's another example where a proper suite of acceptance test assembly's would be really handy - hopefully we'll get round to that one day.
Note the only change is in the final commit - this is just branched off #923 as they will interact. Once #923 is merged, I'll rebase this.
@Learfin, @joeltankam, @VoltanFr, @bouchraRekhadda, @runehalfdan, @webman1 - I gather you've all encountered this issue in one form or another. I've tested against the repro's provided in #916 and #939, but would really appreciate anyone who could test against their particular usecase before merging this. You should be able to download updated packages from the CI build here: https://ci.appveyor.com/project/CharliePoole/nunit-console/builds/38972209/artifacts