Direct Agent Execution#1624
Conversation
manfred-brands
left a comment
manfred-brands
left a comment
There was a problem hiding this comment.
I don't think this new code is part of the testing as I don't see it in build.cake or package-tests.cake but I'm not clear on how the tester is tested.
|
|
||
| private void WriteErrorsFailuresAndWarnings(XmlNode resultNode) | ||
| { | ||
| string resultState = resultNode.GetAttribute("result") ?? ""; |
There was a problem hiding this comment.
"result" node could be passed in like in summary.
| if (resultNode.SelectSingleNode("reason/message")?.InnerText == "One or more child tests had warnings" || | ||
| resultNode.SelectSingleNode("failure/message")?.InnerText == "One or more child tests had errors") |
There was a problem hiding this comment.
Most likely pre-existing, but this is a hard dependency on a string in another repository.
| #endif | ||
| var xmlResult = runner.Run(null, TestFilter.Empty).Xml; | ||
|
|
||
| WriteRunSettingsReport(xmlResult); |
There was a problem hiding this comment.
Would like to see a separate report class.
Is it the idea that the nunit-console calls this class as otherwise we have two report classes.
| #if NETFRAMEWORK | ||
| string agentExe = agentAssembly; | ||
| #else | ||
| string agentExe = Path.ChangeExtension(agentAssembly, ".exe"); |
There was a problem hiding this comment.
Is nunit-console to be supported on non-windows? The NUnitConsole_Linux.sln has not been maintained.
On Linux there is no .exe extension.
There was a problem hiding this comment.
It should continue to be supported. Some linux distros support exe but not all. Let's add an issue for this.
| #else | ||
| const string STANDARD_CONFIG_FILE = "nunit.engine.core.tests.dll.config"; | ||
| #endif | ||
| const string STANDARD_CONFIG_FILE = "nunit.agent.core.tests.exe.config"; |
There was a problem hiding this comment.
Compiling this branch, I see the previous behaviour:
$ find . -name 'nunit.agent.core.tests*.config'
./net462/nunit.agent.core.tests.exe.config
./net8.0/nunit.agent.core.tests.dll.config
|
The DirectTestAgent isn't part of the engine but of the tests. It just provides an exe to allow exercising the direct execution facility in nunit.agent.core. There's only one unit test that uses it right now but once the agents are in separate projects, all their tests will be run through it. We have never tested the agents in isolation before but this makes a start. |
CharliePoole
changed the title
|
This has been dormant for a while, so I'm making it WIP while I rebase and decide how to move it forward. |
CharliePoole
force-pushed
the
issue-1623
branch
from
e38a4a9 to
60508ca
Compare
CharliePoole
force-pushed
the
issue-1623
branch
from
99d4275 to
3b7941a
Compare
|
This is again ready for review. I took most of your suggestions and brought the PR up to date with the latest commits. Since that included the new analyzers, I fixed the analyzer problems. I also added a few package tests to actually make use of the feature. For the moment, I have kept the code duplication for console output. I think some of this code will change once the agents are extensions in their own right. At that point, I'd like to allow it to evolve independently. We'll then have a better idea of exactly what parts can be common and eliminate the duplication. UPDATE: Actually not 100% up to date with version4 but still able to be merged. |
manfred-brands
left a comment
manfred-brands
left a comment
There was a problem hiding this comment.
A few small items which I have fixed in a review commit.
The main issue I have is what I said before, why is the reporting different between direct agent execution and the ConsoleRunner.
Both call runner.Run and print the results.
Why can the AgentDirectRunner not create an ResultReporter for the report?
manfred-brands
left a comment
manfred-brands
left a comment
There was a problem hiding this comment.
Approved with the caveat of your message that the reporting will be re-visited.
|
Thanks for your review. I think it was important to get this odd bit of code in place before I start splitting out all the agents into separate projects. I also agree that something ought to be done about the output before that happens. The duplication in this PR is duplication I just created by design. I copied some code from the console, solely as a way to get us started. I want people to see this feature and give us feedback with ideas about where it could be used and how it should look in the future. Once that happens, we might remove the duplication in several ways...
If I had decided not to have the duplication in the first place, I would be picking one of those things in advance. If this sounds crazy to you, that's OK too. It's something I've done in a lot of places and I've had a lot of strange reactions at first, but it seems to pay off more than not. ;-) |
|
Looks like that alpha.34 label was never removed. I'm a bit tired, so I'll mess directly with the labels on version4 branch in the morning when I'm fresh. |
|
I got a few hours sleep and fixed the tags and did a push --tags -f on version4. Reran the build and it all looks good now. The problem was that the alpha.34 and alpha.35 tags were on detached commits. Latest on myget is now 4.0.0-alpha.35 and I think we're now good for future runs. |
2 participants
Fixes #1623