-
Notifications
You must be signed in to change notification settings - Fork 151
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
Run .NET Core Tests from NUnit3 Console #937
Conversation
Unable to figure out why the Azure builds are failing. Windows job can't find Linux and MacOs jobs say that the net-4.0 framework isn't available. Not being an Azure guy, I don't even know where to look. |
Not sure if the CI has been re-run since your comment Charlie, but looks like the Windows build is ok. Looks to me like the Mono tests are all failing to start up an agent on Linux/OSX. Not sure what's in this PR that would affect that, however... |
The error in building windows continues to appear intermitently, i.e. failure to locate I'm working on the hypothesis that nuget is choking on the odd setup we have for the two agent projects: two projects in one directory with separate intermediate output folders, I'll run an experiment, separating the two projects into two different folders and replicating the code. |
9e39007
to
a6d3e9e
Compare
That worked! The latest commit builds under Windows on both AppVeyor and Azure. The Linux and MacOs builds are still failing on Azure, but not from the same cause. |
0c8666d
to
d5d706e
Compare
d5d706e
to
eda6812
Compare
a24190f
to
3e3b43d
Compare
The result is that AppVeyor doesn't seem to have the X86 version of dotnet.exe installed. We could probably install it in our script, but I'd rather save that for later. Instead I make a check for it's availability before trying to run the test, which is more robust. After this last commit, the only remaining issue is the msi build. |
@ChrisMaddock OK, last bit is done and this is ready for final review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this all coming together Charlie! Just a few niggles at this point. 🙂
Just checkpointing the review here as GitHub's gone a bit funny - still a bit more to go through...
<appSettings> | ||
<add key="test.setting" value="54321"/> | ||
</appSettings> | ||
</configuration> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is included within the "add agent to msi" commit - is it included accidentally? I'm not sure what the purpose of this is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to add it to make all tests pass. There's a test that reads it. In fact, this should eventually be cleaned up and we should stop using .config
files entirely for .NET Core as a part of reworking RuntimeFramework. .runtimeconfig.json
is where this should end up.
Hard to figure out the boundaries of a PR... i.e. where do you stop and create a new issue. Personally, I'd prefer not to deal with this one while we still allow --inprocess
because that makes RuntimeFramework more complicated. I do think that RuntimeFramework is a good place for me to exercise my preference for tiny PRs. 😃
ProcessorArchitecture="msil" | ||
Source="$(var.InstallImage)bin/agents/netcoreapp3.1/nunit-agent.dll" /> | ||
<File Id="nunit_agent.runtimeconfig.json" | ||
Source="$(var.InstallImage)bin/agents/netcoreapp3.1/nunit-agent.runtimeconfig.json" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not totally clear on what is and isn't required for .NET Core, but I think this should also include nunit-agent.deps.json.
I was also wondering about nunit-agent.dll.config, but I think is actually netfx specific, so it should probably be removed entirely from the netcoreapp agent? I'm not sure how to do that off the top of my head, will give it a go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that nunit-agent.deps.json
is only for use in building the project. It causes nunit-agent.runtimeconfig.json
to be generated. If that's wrong, I can add it. Do you have different info? At this point all the tests pass using all the packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, ok. I always thought it should be as it details where to load dependencies from at runtime. I'm struggling to find any formal documentation however, or any clear answer elsewhere.
This page seems to be the closest there is. From my reading of that, for a framework-dependent deployment as ours is, the file should be included. What do you think?
https://github.com/dotnet/cli/blob/master/Documentation/specs/runtime-configuration-file.md
msi/nunit/engine-files.wxi
Outdated
<Component Id="NUNIT_AGENT_ADDINS_NETCORE31" Location="local" Guid="21D2D9FB-A57D-4B02-92A0-6697674FB869"> | ||
<File Id="nunit.agent.netcore31.addins" Source="$(var.InstallImage)nunit.agent.addins" /> | ||
</Component> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Component Id="NUNIT_AGENT_ADDINS_NETCORE31" Location="local" Guid="21D2D9FB-A57D-4B02-92A0-6697674FB869"> | |
<File Id="nunit.agent.netcore31.addins" Source="$(var.InstallImage)nunit.agent.addins" /> | |
</Component> |
I think this addins file points to a dir of netfx extensions, none of which can be loaded by the netcoreapp31 agent. Shall we just leave this out for now? We might want to package netcoreapp-compatible extensions with the engine in future, but they'll probably need to be in a different subdir anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll take a look. In fact, this directory should only used for the .NET Core 3.1 agent, so I may be able to just change the content of either the addins
file or the directory itself.
nuget/engine/nunit.engine.nuspec
Outdated
|
||
<file src="bin/agents/netcoreapp3.1/nunit-agent.dll" target="contentFiles/any/agents/netcoreapp3.1" /> | ||
<file src="bin/agents/netcoreapp3.1/nunit-agent.dll.config" target="contentFiles/any/agents/netcoreapp3.1" /> | ||
<file src="bin/agents/netcoreapp3.1/nunit-agent.runtimeconfig.json" target="tools/agents/netcoreapp3.1" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<file src="bin/agents/netcoreapp3.1/nunit-agent.runtimeconfig.json" target="tools/agents/netcoreapp3.1" /> | |
<file src="bin/agents/netcoreapp3.1/nunit-agent.runtimeconfig.json" target="contentFiles/agents/netcoreapp3.1" /> |
I think this one's been missed, Charlie. (I also don't know why we use contentFiles instead of tools here, but keeping it consistent seems safest! 😅)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially tried to keep it consistent, but I got failures when I did that. However, I can now think of another reason for that, so I'll try again. If this were for 4.0, I'd say just drop the whole contentFiles
thing unless somebody can say why.
src/NUnitEngine/nunit.engine/Services/RuntimeFrameworkService.cs
Outdated
Show resolved
Hide resolved
Ok - I think I've covered everything here. I've resolved a few discussions that I think are sorted, and unresolved a couple that I think were missed. I've added a few new comments, almost entirely re: the new changes. I think I've also responded to all still-open conversations. The x86 changes look good - glad finally we've got some tests around that too! In future, I'd like to add tests to mock-assembly which only pass if they are running on the targeted framework/architecture, so we can smoke test that functionality too. Let me know if there's anything I've missed - let's get this merged! 😁 |
I answered some questions and resolved a couple of items but I'll go
through all the items again. It's really hard to track all this with so
many changes, but I'm keeping a page open where I've marked all the ones I
no longer want to see as "viewed".
WRT testing that it really runs under the targeted runtime and
architecture...
I'd like to do that too, but when I took a casual look at the idea, it
seemed as if I would have to add more info to TestResult.xml to do that.
How about if I do whatever can be done without changing the XML format and
then trying to do more when we incorporate this in the 4.0 branch?
…On Sun, May 2, 2021 at 8:07 AM Chris Maddock ***@***.***> wrote:
Ok - I *think* I've covered everything here.
I've resolved a few discussions that I think are sorted, and unresolved a
couple that I think were missed. I've added a few new comments, almost
entirely re: the new changes. I think I've also responded to all still-open
conversations.
The x86 changes look good - glad finally we've got some tests around that
too! In future, I'd like to add tests to mock-assembly which only pass if
they are running on the targeted framework/architecture, so we can smoke
test that functionality too.
Let me know if there's anything I've missed - let's get this merged! 😁
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#937 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAUN7BPDYUSNIJ3UWHEDWMLTLVTBPANCNFSM423UETAQ>
.
|
Thanks Charlie - and it is a lot to keep track of. Hopefully we'll catch most things between us!
Ah - I was thinking more simply a test which fails when it's run on e.g. .NET 2.0, and using pre-compilation symbols to make sure the right version is included in the right assembly. That's what I do in my manual tests before releases. 🙂 |
If I understand you, that would work if the test can run, e.g. net20 on net40. But if the test can't run at all, the package tests typically need to examine the TestResult file to find out why it failed. That's because we keep running subsequent tests rather than throwing immediately. I may be able to figure out a way to recognize that the test failed because the agent couldn't load it though. |
@ChrisMaddock This PR and reviews and comments on reviews etc. is getting a bit hard to follow. So to checkpoint where we are, here's a list of items you brought up, which I believe still need to be dealt with...
Let me know if I've missed something. 😃 |
That looks about right to me! 👍
|
@ChrisMaddock Regarding (5) I probably didn't make myself clear. It's not a rebase error. I saw that you had removed them and I had to put them back in to the package tests for .NET Core to pass. There was no failure before because we didn't have such a test or even a .NET Core agent to fail! That said, I'm taking on trying again without them and seeing if this interacts with the presence or absence of `nunit-agent.deps.json'. I think this is something we really need to understand better. |
Should be ready to go now. I think we have a number of packaging decisions to make for 4.0, however. |
Great, thanks for getting those sorted! Afraid there's a couple of things in the last commit I still think need looking at.
Let me know if there's anything we need to discuss further - otherwise please go ahead and merge once the changes are in. Thanks for all your work getting it sorted! 😄 |
I'm afraid I thought I was doing what you wanted without really understanding why you wanted it. I'll take the blame for that choice. In future, I think it would help to have failing scenarios that force us to add something to the packages. After all, that's why we have package tests. I'll make the changes in the morning. |
Great - thanks for this Charlie! One step closer to where we want to be! 😁 |
Another step toward issue #923.
This PR adds TCP as a transport and uses it to run tests under .NET Core.
A .NET Core 3.1 agent has been added. We'll have to decide whether we want others as separate agents. A single package test has been added to exercise the agent. It is run for the NuGet, Chocolatey and Zip packages of the console runner.
The MSI package does not yet include the .NET Core agent. I made a start on this but backed out the changes when I saw that we need to make some decisions about the msi package. I think this will be easier to do if there's a separate PR for it. This one is pretty big as it is.