Further changes to support use of nunit.extensibility.dll #1341

CharliePoole · 2025-10-25T05:39:07Z

CharliePoole · 2025-10-25T06:10:06Z

@roald Next steps in use of the NUnit.Extensibility assembly.

I'm ready to sleep now, so I'll ask you to follow up with review if you have time. Once you have left your comments, feel free to fix things as you see the need with another commit to the same branch. I'll then take the baton and continue in the same way.

I still have not got the agent selection to work correctly but I have some changes underway for the next iteration. I think the problem is that I was completely re-creating the entire structure each time you opened the menu. I'm now separating the creation of the menu from going through it and enabling, disabling, checking and unchecking items. Once you are running the gui, the available agents don't change so there it is not really necessary to create it each time. This work is partly done but I'm too tired to continue right now

rowo360 · 2025-10-25T08:56:59Z

src/GuiRunner/TestModel/TestModel.cs

-        public IList<string> AvailableAgents { get; }
+        // Since we generate a new name when agents are wrapped,the available agent list must created dynamically.
+        public IList<string> AvailableAgents =>
+            [.. TestEngine.Services.GetService<ITestAgentProvider>().GetAvailableAgents().Select((a) => a.AgentName)];


We could use also Services.GetService<> instead of TestEngine.Services.GetService<>(), don't we?

You may notice I removed caching entirely from Services. I'm of two minds about keeping the class, although a very simple forwarding property Services => TestEngine.Services may be convenient. Or possibly EngineServices` to make it clear that the engine is doing something for us. What do you think.

In general, I am feeling that the TestModel has become ore complicated than necessary, but that's for another issue I think. :-)

rowo360 · 2025-10-25T09:07:45Z

src/GuiRunner/TestCentric.Gui/Presenters/AgentSelectionController.cs

            if (agentMenu.Enabled)
            {
-                agentMenu.MenuItems.Clear();
+                //agentMenu.MenuItems.Clear();


I assume that this is precisely the code which you mentioned that doesn't quite work yet. Currently the menu items for the agents are added over and over again when the pop-up menu is opened (and the dummy entry is displayed)

From my point of view it's not critical if we build up the menu every time anew. Of course, it's true that the entries won't change, but before it gets too complicated, you can also take the easy route. Unfortunately I noticed that the checked item is not retained when building up anew, so some effort is required anyhow 😃
If you agree I can try to solve this menu item stuff in whatever way you prefer.

I can see that what I checked in was not what I intended to check in. In the GUI, I often do simple experiments, e.g. if I remove this line, what will I see? This was one of those and in fact it's how I realized that it's not necessary to construct everything from scratch each time. I removed it but must not have committed the change before I pushed.

I already have the code to a point where there are two methods, one to construct and one to update based on what is being loaded, and it seems more understandable to me. I'll push it so you can see it.

@rowo360 It turns out that NUnit itself is returning a short agent name... i.e. "Net80AgentLauncher" rather than "NUnit.Engine.Agents.Net80AgentLauncher." I'll do a workaround for now.

rowo360 · 2025-10-25T09:16:19Z

src/GuiRunner/TestModel/TestModel.cs


-            AvailableAgents = new List<string>(
-                Services.TestAgentService.GetAvailableAgents().Select((a) => a.AgentName));
+            Services.GetService<IExtensionService>().InstallExtensions();


I just wondering if InstallExtensions() is the responsiblity of the TestModel or can this task be executed by TestEngine.Initialize() which creates all services and and start them?

Now that we have the ability for the application using the engine to continue to initialize using additionally directories, it seems convenient for the application to use this call in order to tell the engine when to finalize all the extensions. The code I removed for automatically forcing finalization when certain calls are made was leading to hard-to-understand failures.

For the longer term, I think it would be better if the engine could accept adding new directories and merging them in with what had already been recognized but that's a very big internal change. Also, if we are going to abandon the TestCentric engine, it's a very big internal change in NUnit and requires discussion with that team.

So, using InstallExtensions to indicate that initialization can be completed is a way to keep the change in our hands for now.

rowo360 · 2025-10-25T10:09:37Z

src/TestEngine/testcentric.engine/Services/TestAgency.cs


        private const int NORMAL_TIMEOUT = 30000;               // 30 seconds
        private const int DEBUG_TIMEOUT = NORMAL_TIMEOUT * 10;  // 5 minutes
+        private const string AGENT_LAUNCHERS_PATH = "/TestCentric/Engine/AgentLaunchers";


This comment is not about this specific code line, but more a general question regarding ExtensionPoints: we define our own ExtensionPoints which are mapped however to NUnit extension types. With that we managed to eliminate our own extension types, which is perfect of course! I imagine that we want to go further later on and eliminate also these ExtensionPoints?
Well, I stumpled upon it thinking about the 'TestCentric' term in this constant and how it's currently working (but I spot the definition in EngineExtensionPoints - so, it's clear now)

Yes... it seems that each application has to define it's own extension points. Using TypeExtension to define them only works cleanly if the same application (i.e. NUnit Engine) both defines and implements them. I experimented with using those extension points, but I like the use of Extension better.

Of course, this implies that all extension points - at least all those being used at one time - should have a unique path. Therefore, beginning the path with the name of the application, possibly followed by the component, seems like the best pattern.

rowo360 · 2025-10-25T10:18:16Z

src/TestEngine/testcentric.engine/Services/TestAgentService.cs

        }

-        #endregion
-


Hmm, it's difficult to add a comment for existing (not changed) code part. But maybe it work 😄
In this class in method StartService this code line looks suspicious:
ITestAgentProvider testAgency = ServiceContext.GetService<TestAgency>();

We are requesting a service of type TestAgency, but implicitly assume that it's a ITestAgentProvider.

rowo360 · 2025-10-26T09:42:56Z

src/GuiRunner/TestCentric.Gui/Presenters/AgentSelectionController.cs

+                    string itemTag = item.Tag as string;
+                    item.Enabled = itemTag == "DEFAULT" || agentsToEnable.Contains(itemTag);
+
+                    item.Click += (s, e) =>


I propose to move the Click event registration to method PopulateMenu: otherwise we are registering over and over again when opening the menu. I didn't notice any totally bad behavior, because the !item.Checked condition is only fulfilled on the first invocation. But nonetheless the event notification is invoked multiple times.

rowo360 · 2025-10-26T09:53:49Z

src/GuiRunner/TestCentric.Gui/Presenters/AgentSelectionController.cs

-                    defaultMenuItem.Checked = true;
-                    //packageSettings.Remove(SettingDefinitions.SelectedAgentName);
-                }
+            void EnsureSingleItemChecked(ToolStripMenuItem itemToCheck)


This code section reminds me that we have a class CheckedToolStripMenuGroup which takes care that exactly one item of a set of ToolStripMenuItems is checked. So the presenter class would work basically with an ISelection.

Hmm, I think it's not necessary to change this entire code to that approach, now. But I just want to mention it 😄

This code doesn't make a lot of use our Element structure, except for IPopup to kick things off. It would be cleaner to use it but really only for our own satisfaction.

CharliePoole · 2025-10-26T10:23:41Z

@rowo360 I'm only out of bed for a short bit here at 3AM, so I won't be working on this stuff for another 5 or 6 hours. If you wish to make the changes yourself, either of the points you raise would be good to do. We haven't yet tried working in the same branch and PR together, so it might be interesting to try it.

In either case, i.e. you do it now or I when actual morning arrives, I think I'd like to get the point about multiple click event registration fixed and then merge this particular PR. I have two candidates for the next step.

Remove the project loader wrapper, which works for NUnit projects but not for VS projects. I have no plans to make it work for the latter anyway and our new approach will make wrappers unnecessary. We have to keep the Agent wrapper of course, so we can keep running tests. :-) This is a pretty quick change.
The rather large change of using the NUnit versions of PackageSetting, PackageSettings, SettingDefinition, SettingDefinitions and TestPackage. I actually have the first two done. PackageSettings is the first one where we have code that NUnit doesn't. I may use extension methods for the time being until/unless I can get NUnit fixed. PackageSettings is the trickiest because we use the same name for different purposes.

After those two PRs, I'll be ready to close this issue but continue the process in smaller individual issues.

Do you have time for a video chat sometime after 10am?

rowo360 · 2025-10-26T16:02:56Z

I commit a fix for the click event registration. The code is in a separate method because it's required for the defaultItem and all items for the available agents.
Please review especially this line which I removed from the OnClick handler:
item.Enabled = agentsToEnable.Contains(itemTag);

Well, I don't have the list of agentsToEnable on hand and I had the impression that the Enable state won't change on clicking one item, am I right?

rowo360 · 2025-10-26T16:10:49Z

@CharliePoole :
A video call would be great - unfortunately we're having guests over tonight, so it doesn't fit best today. But I'm free in the evenings next week from Monday to Friday 😄.

CharliePoole · 2025-10-26T16:13:41Z

The enabling of different agents can only happen if you make a change to the assembly, for example, changing it from .NET 8.0 to 9.0. I'll review the code and see if we can provide the info easily. If not, this is something that can become a future issue.

UPDATE: I misunderstood, the OnClick handler is exactly where it needs to be!

I'll contact you offline about scheduling a chat.

CharliePoole added 2 commits October 24, 2025 15:53

Further changes to support use of nunit.extensibility.dll

cc03ec5

Update to latest engine version

3fd3fe3

CharliePoole requested a review from rowo360 October 25, 2025 05:51

rowo360 reviewed Oct 25, 2025

View reviewed changes

rowo360 approved these changes Oct 25, 2025

View reviewed changes

CharliePoole added 2 commits October 25, 2025 10:21

Fix agent selection menu

6852337

Make sure that the selected agent is actually used!

8fb1ac7

CharliePoole requested a review from rowo360 October 25, 2025 21:54

rowo360 reviewed Oct 26, 2025

View reviewed changes

rowo360 approved these changes Oct 26, 2025

View reviewed changes

Agent selection menu - register click event only once

330dcce

CharliePoole merged commit ac60d7c into main Oct 26, 2025
2 checks passed

CharliePoole deleted the issue-1336b branch October 26, 2025 17:00

Uh oh!

Further changes to support use of nunit.extensibility.dll #1341

Further changes to support use of nunit.extensibility.dll #1341

Uh oh!

Conversation

CharliePoole commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CharliePoole commented Oct 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rowo360 Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rowo360 Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CharliePoole commented Oct 26, 2025

Uh oh!

rowo360 commented Oct 26, 2025

Uh oh!

rowo360 commented Oct 26, 2025

Uh oh!

CharliePoole commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

CharliePoole commented Oct 25, 2025 •

edited

Loading

rowo360 Oct 25, 2025 •

edited

Loading

rowo360 Oct 25, 2025 •

edited

Loading

CharliePoole commented Oct 26, 2025 •

edited

Loading