Skip to content

Add new interfaces and enable use of launchers internally #1674

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

Open
wants to merge 3 commits into
base: version4
Choose a base branch
from

Conversation

CharliePoole
Copy link
Member

Fixes #1673

As explained in the issue, some of this is temporary, allowing us to use the same concept of a "launcher" for internal agents that will be used for pluggable agents. After all agents become pluggable extensions, the launcher code will disappear from the engine itself. The plan for transitioning is...

  1. Add one new pluggable agent, i.e. .NET 9.0.
  2. Convert the .NET 8.0 agent to be pluggable.
  3. Ditto for the .NET 4.6.2 agent.

@CharliePoole CharliePoole requested a review from veleek April 29, 2025 00:43
@CharliePoole
Copy link
Member Author

@veleek I thought this wold be a good way for you to get familiar with the v3 vs v4 approach to launching agents.

@CharliePoole
Copy link
Member Author

@veleek FYI I'm going to have a second commit on this. You can either wait for that or start reviewing the first commit, whichever seems convenient.

@CharliePoole
Copy link
Member Author

@nunit/engine-team I've greatly increased the scope of issue #1673 so I've asked several of you to review this. Those not named are, of course, also welcome to review or comment.

This is a key new feature and will put us very close to being able to release a beta. Whatever time you can give will be helpful.

Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like Process to be abstracted, especially if you want to support InProcess and RemoteProcess agents. See further remarks embedded.

I assume that the existing AgentProcess and AgentProcessTest classes will be deleted once this is finished.

Not sure if we want to do #1660 first as this PR touches on cases where a different transport is used.
If you keep RemotingTransport should all that code be in the Net462Agent as the only one using it.

Comment on lines +156 to 158
agentProcess.Exited += (sender, e) => OnAgentExit(agentProcess);

agentProcess.Start();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed this should be abstracted into a LocalProcessAgent

return _agentStore.IsAgentProcessActive(agentId, out process);
}

private void OnAgentExit(Process process)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This handler should be in the LocalProcessAgent

@CharliePoole
Copy link
Member Author

Yes, I've been living with the tension created by GetAgent returning a Process, figuring I'd reconcile it at some point. I think this issue is the place to do it but it may be a commit or two away.

@CharliePoole
Copy link
Member Author

Regarding issue #1660, I had thought to do it first, but @veleek volunteered to look at it so we're working in parallel. I'm hoping to rebase on his changes at some point.

@CharliePoole
Copy link
Member Author

@manfred-brands I made a few off-the-cuff comments. I'll come back to these for actual action in the next commit.

@CharliePoole
Copy link
Member Author

@nunit/engine-team I've greatly increased the scope of issue #1673 so I've asked several of you to review this. Those not named are, of course, also welcome to review or comment.

This is a key new feature and will put us very close to being able to release a beta. Whatever time you can give will be helpful.

@CharliePoole
Copy link
Member Author

CharliePoole commented May 1, 2025

The second commit does two things:

  1. Moves the implementation of the agent launcher into an abstract class. Our individual launchers only need to override a few members.

  2. Applies most of @manfred-brands suggested code changes.

The exception to the code changes is the proposal that we make the launcher interface be about agents, abstracting from processes. This is where I'd like to go but it's a big change, so I'll take a shot at it in the next a later commit.

I can see two ways to do this:

  1. Let the AgentLauncher start the process and then return an ITestAgent that wraps it, as is currently done within TestAgency. That means the launcher will also have to set up a handler for the Exited event. This implies that the AgentLauncher as it now exists would become a LocalAgentLauncher as suggested in the review.

  2. Provide a way to get the process from an AgentLauncher. TestAgency would get the process that way and then proceed just as it now does. This is not unreasonable since TestAgency only handles LocalProcess agents. This is probably a bit simpler, so I may try it first.

@CharliePoole
Copy link
Member Author

The third commit is mostly a major file reorganization. Launchers are moved to a separate project, which is how they will be when they are extensions. This creates some odd warnings due to incompatible references that are not actually used by the code. These will go away when I remove the launchers and agents.

I renamed AgentLauncherBase to LocalProcessAgentLauncher', which is exactly what it is. We'll eventually have an InProcessAgentLauncherand probably aRemoteProcessAgentLauncher`.

Made some other changes due to @veleek 's comments.

@CharliePoole
Copy link
Member Author

I'm moving away from this PR for a bit in order to work on the two independent agent projects. Once they are set up, I'll be able to remove the agent code from this project and have it depend on the two agent packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants