V4 Codebase Should be Null-Enabled #1539

CharliePoole · 2024-12-15T16:45:28Z

I'm treating this as a prerequisite for re-booting work on version 4. I observed that null-enabling the framework code brought up many issues, so it may be desirable to do this one assembly at a time, in separate PRs.

This depends on updating the language level, which I'm doing in a separate issue.

manfred-brands · 2024-12-29T12:03:37Z

@CharliePoole Converting existing code to nullable is not simple if done right.

It heavily depends on the code at hand.
If a class has various fields that are set in certain methods and used in others it could result in a lot of tests on null.

Removing the need for this might mean splitting functionality in separate classes.

How far do you want to take this?

I can have a look and do an initial investigation.

What project do you think I should start with?

mikkelbu · 2024-12-29T12:15:20Z

I was considering starting with determining how the different project relates and then start with the "bottom" (the project that only has no/external dependencies).

I think we should just start with the quick solutions - e.g. marking certain areas/methods as ignored via nullable disable and similar and then improve on this later on. Otherwise, I fear that this could take a very long time.

I had originally considered volunteering for giving a hand on this, but I lack the time at the moment (or at least proper time chunks and not just 15-25 minute blocks now and then).

CharliePoole · 2024-12-29T17:10:02Z

@manfred-brands Thanks! I'm glad to have you working on this. I'll help by asking naive questions. :-)

Seriously, I've experimented only a little with nullable types, so I prefer not to do this myself in the engine. One bit of general guidance: I suggest not trying to get each project "done" all at once. If there are particularly troublesome areas, @mikkelbu 's suggestion of ignoring them for a time makes sense to me. The only risk is that we forget to return and fix it, so let's not do that... we can leave behind some cleanup issues as we go or scan the entire repo and make a list at the end.

The dependencies will change as development continues, so I'll show both the current ones and those planned.

Current Dependencies

nunit4-console
  => nunit.engine.api
  => nunit.engine (1)

nunit.engine
  => nunit.engine.api
  => nunit.engine.core  =>  nunit.engine.api

nunit-agents (2)
  => nunit.engine.api
  => nunit.engine.core

Planned Dependencies

nunit4-console
  => nunit.engine.api

nunit.engine
  => nunit.engine.api
  => nunit.extensibility (3)

pluggable agents
  => nunit.engine.api
  => nunit.agent.core (4)
  => nunit.extensibility (3)

Notes

At the moment, the runner depends on the engine itself as well as the api. This dependency crept in as a workaround to resolve a problem but I'm going to try to remove it. [See Eliminate reference to nunit.engine by nunit-console runner #1576]
There are currently five separately named agents, all with the same dependencies. Eventually, all agents will be pluggable engine extensions each in it's own repo.
See Separate assembly for NUnit extensibility #1049
The plan is to no longer have any refs common to both the engine and the agents except for nunit.engine.api. Types currently in nunit.engine.core will be moved to either nunit.agent.core or nunit.engine itself. I need to create an actual issue for this! [See Isolate agents from the engine itself #1578]

As for where to start, nunit.engine.api seems sensible. It mostly has interfaces, with two reference types used as arguments. So most of the work will be in the classes that implement the interfaces and those in other projects that reference them. The TestPackage class has the particular difficulty that it uses a null Name property to signify that it's the unnamed top-level package, i.e. the one created from the command-line. This is something that may require refactoring but it would be good to get it out of the way at the start.

How does that sound?

manfred-brands · 2024-12-30T11:53:30Z

@CharliePoole Apologies for my list of comments, I know a lot of this code has been there a long time.

I started on the api/core assemblies and came to a few observations/questions:

TestEngineActivator has different CreateInstance signature and implementation for .netstandard2.0 vs .net 462/
a. For the first the overload to specify a minimum version doesn't exist.
b. For the second, even though it already has the Assembly loaded, it calls a method to load it again by name. I don't see any benefit to the CreateInstanceFromAndUnwrap method on the current domain vs Activator.CreateInstance especially as the first calls the second
NUnitEngineUnloadException a user might have to check multiple properties to find the underlying exception. The standard way would be to pass the list of exceptions to AggregatedException and then pass that as the inner exception. But that is a breaking change.
In ExtensionManager.FindExtensionsInAssembly has a local assemblyTargetFramework which is ever only set to null, but its properties are used in exception and log messages. Note this is also the case in the main branch. I see several changes to this code part, but not sure the intention. There was an TargetFramework property on IExtensionAssembly but that is commented out as the definition of RuntimeFramework is in nunit.engine and hence not available in nunit.engine.api.
The result of this is that ExtensionNode.TargetFramework is always null.
TestCentric.Metadata could do with a nullable enabled version. So that GetNamedArgument is marked as object?.
I started on that, but with that project everything could be null or is set indirectly. So that will take a while.
ExtensionNode.Path says it must be unique, but as it is not set by a factory it can be null for all of them or multiple instances can have the same path. There is no code to check for uniqueness. The only way to guarantee that is by creating a factory that has a dictionary of all paths and make an internal constructor that also expects the path and remove the public setter. But as the class is public that would be breaking.
ExtensionNode.CreateExtensionObject can return null, but the consuming code doesn't expect that.
This also has two different way to create an instance as mentioned in 1.
TestPackage.FullName is only set for sub-packages. But as it is there is only one type. Having a separate types would be nicer.
Several places open a RegistryKey without disposing it.
The TestAgentRunner checks there is a single package, but in the Load it does another selection on !subPackage() and a for loop. If the runner only supports a single package then the loop in Load is not needed.
TestAgentRunner.TestDomain is a property used in the base class, but expected to be set in a derived class. Ideally this should be passed in a constructor as we now have no way of guaranteeing it is set by a (3rd party) derived class.
I added a Guard.OperationValid to the Load method.
InternalTraceWriter.Write(string? value) calls base.Write instead of _writer.Write. Eventually that will call the Write(char) overload but the same could be said for the WriteLine overload. Seems inconsistent.
In TestAgentRemotingTransport ITestEngineRunner implemenation the user must call CreateRunner before calling any of the interface methods. There is no way to force that is actually done unless we return a wrapper class with a non-null runner. Again a breaking change. For now I added a (new) ShouldNotBeNull Guard.
Another thing is that if users call CreateRunner twice we might have a "leak". I added a conditional Dispose before creating a new one.
TcpChannelUtils.CreateTcpChannel had a limit parameter who's usage is commented out. Should it be removed or re-instated?
NotRunnableFrameworkDriver relies on derived classes to set runstate, result and label. A better solution would have been virtual properties for these field with implementations in the derived classes. But again a breaking change.
Same for NotRunnableTestRunner but here the derived class doesn't set them.
IFrameworkDriver depends on users calling Load first. A design where Load return a callable object would fit better with nullability.
NUnit3FrameworkDriver passes in a CallbackHandler to the actions and depends on that method calling this once. Could these method not return a string instead of calling a callback? We have now no guarantee that the api will call that handler. Whilst the NUnitNetCore31Driver calls the method that return values. Why the difference? Can the framework driver not be the same?
Path.GetDirectoryName is very annoying, so is DirectoryInfo.Parent.
nunit.engine.core uses space and nunit.engine.core.test uses tabs.
There are various places that test a variable for null then call log.Error but then continue accessing that null variable.
I cannot find that Logger.Error aborts.

if (method == null)
    log.Error($"Method {methodName} was not found in {_frameworkControllerType.Name}");
log.Debug($"Executing {method.DeclaringType}.{method.Name}");

CharliePoole · 2024-12-30T14:56:15Z

@Manfred Brands ***@***.***> I'm just getting up and you're probably going to bed if you haven't already. Some general comments now in case you see them. I'll go through the details during the day. You're right. Some of the code is quite old. Some has suffered from inattention and some from too much attention, i.e. from attempts to pull it in conflicting directions. The `nunit.core` assembly is probably one of the worst and it's scheduled to disappear so maybe we don't want to start with it. It does seem that this may need to be incremental at a lower level than one assembly at a time. Or top-down rather than bottom up? I'm thinking in terms of protecting the higher level callers from the mess underneath as a start. Anyway, this may make more sense as I get into the detaills. See you in the morning. :-)

…

On Mon, Dec 30, 2024 at 3:53 AM Manfred Brands ***@***.***> wrote: @CharliePoole <https://github.com/CharliePoole> Apologies for my list of comments, I know a lot of this code has been there a long time. I started on the api/core assemblies and came to a few observations/questions: 1. TestEngineActivator has different CreateInstance signature and implementation for .netstandard2.0 vs .net 462/ a. For the first the overload to specify a minimum version doesn't exist. b. For the second, even though it already has the Assembly loaded, it calls a method to load it again by name. I don't see any benefit to the CreateInstanceFromAndUnwrap method on the current domain vs Activator.CreateInstance especially as the first calls the second <https://referencesource.microsoft.com/#mscorlib/system/appdomain.cs,ab93c6b78486b065> 2. NUnitEngineUnloadException a user might have to check multiple properties to find the underlying exception. The standard way would be to pass the list of exceptions to AggregatedException and then pass that as the inner exception. But that is a breaking change. 3. In ExtensionManager.FindExtensionsInAssembly <https://github.com/nunit/nunit-console/blob/version4/src/NUnitEngine/nunit.engine.core/Extensibility/ExtensionManager.cs#L479> has a local assemblyTargetFramework which is ever only set to null, but its properties are used in exception and log messages. Note this is also the case in the main branch. I see several changes to this code part, but not sure the intention. There was an TargetFramework property on IExtensionAssembly but that is commented out as the definition of RuntimeFramework is in nunit.engine and hence not available in nunit.engine.api. The result of this is that ExtensionNode.TargetFramework is always null . 4. TestCentric.Metadata could do with a nullable enabled version. So that GetNamedArgument is marked as object?. I started on that, but with that project everything could be null or is set indirectly. So that will take a while. 5. ExtensionNode.Path says it must be unique, but as it is not set by a factory it can be null for all of them or multiple instances can have the same path. There is no code to check for uniqueness. The only way to guarantee that is by creating a factory that has a dictionary of all paths and make an internal constructor that also expects the path and remove the public setter. But as the class is public that would be breaking. 6. ExtensionNode.CreateExtensionObject can return null, but the consuming code doesn't expect that. This also has two different way to create an instance as mentioned in 1. 7. TestPackage.FullName is only set for sub-packages. But as it is there is only one type. Having a separate types would be nicer. 8. Several places open a RegistryKey without disposing it. 9. The TestAgentRunner checks there is a single package, but in the Load it does another selection on !subPackage() and a for loop. If the runner only supports a single package then the loop in Load is not needed. 10. TestAgentRunner.TestDomain is a property used in the base class, but expected to be set in a derived class. Ideally this should be passed in a constructor as we now have no way of guaranteeing it is set by a (3rd party) derived class. I added a Guard.OperationValid to the Load method. 11. InternalTraceWriter.Write(string? value) calls base.Write instead of _writer.Write. Eventually that will call the Write(char) overload but the same could be said for the WriteLine overload. Seems inconsistent. 12. In TestAgentRemotingTransport ITestEngineRunner implemenation the user must call CreateRunner before calling any of the interface methods. There is no way to force that is actually done unless we return a wrapper class with a non-null runner. Again a breaking change. For now I added a (new) ShouldNotBeNull Guard. Another thing is that if users call CreateRunner twice we might have a "leak". I added a conditional Dispose before creating a new one. 13. TcpChannelUtils.CreateTcpChannel had a *limit* parameter who's usage is commented out. Should it be removed or re-instated? 14. NotRunnableFrameworkDriver relies on derived classes to set *runstate*, *result* and *label*. A better solution would have been virtual properties for these field with implementations in the derived classes. But again a breaking change. 15. Same for NotRunnableTestRunner but here the derived class doesn't set them. 16. IFrameworkDriver depends on users calling Load first. A design where Load return a callable object would fit better with nullability. 17. NUnit3FrameworkDriver passes in a CallbackHandler to the actions and depends on that method calling this once. Could these method not return a string instead of calling a callback? We have now no guarantee that the api will call that handler. Whilst the NUnitNetCore31Driver calls the method that return values. Why the difference? Can the framework driver not be the same? 18. Path.GetDirectoryName is very annoying, so is DirectoryInfo.Parent. 19. nunit.engine.core uses space and nunit.engine.core.test uses tabs. 20. There are various places that test a variable for null then call log.Error but then continue accessing that null variable. I cannot find that Logger.Error aborts. if (method == null) log.Error($"Method {methodName} was not found in {_frameworkControllerType.Name}"); log.Debug($"Executing {method.DeclaringType}.{method.Name}"); — Reply to this email directly, view it on GitHub <#1539 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAUN7BPSWHES5QWJD7SGDWD2IEX5BAVCNFSM6AAAAABTUWQFYSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRVGM4DGOBTGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

CharliePoole · 2024-12-30T17:06:02Z

I'm splitting up my comments, so you get them sooner and we can deal with changes individually. In some cases,
I won't comment here, but only in the PR. In others both places.

This set relates to the api assembly...

1. TestEngineActivator has different CreateInstance signature and implementation for  .netstandard2.0 vs .net 462/
   a. For the first the overload to specify a minimum version doesn't exist.
   b. For the second, even though it already has the `Assembly` loaded, it calls a method to load it again by name. I don't see any benefit to the CreateInstanceFromAndUnwrap method on the current domain vs Activator.CreateInstance especially as the [first calls the second](https://referencesource.microsoft.com/#mscorlib/system/appdomain.cs,ab93c6b78486b065)

What you are running into here is the fact that the .NET Core build of the runner lacks many features of the .NET Framework version. Historically, one of those was the ability to examine a set of available engines and decide on the best one to use. Frankly, it's a feature we imagined we needed but never did. The engine is now required to be bundled with any runner that uses it or at least treated as a dependency so it's present.

I made an online comment suggesting a simplified implementation of TestEngineActivator

2. `NUnitEngineUnloadException` a user might have to check multiple properties to find the underlying exception. The standard way would be to pass the list of exceptions to `AggregatedException` and then pass that as the inner exception. But that is a breaking change.

Breaking is OK at this point. Break away. :-)

CharliePoole · 2024-12-30T18:54:28Z

TestPackage.FullName is only set for sub-packages. But as it is there is only one type. Having a separate types would be nicer.

Yes, I agree. Separate issue or here?

CharliePoole · 2024-12-30T19:00:09Z

TestCentric.Metadata could do with a nullable enabled version. So that GetNamedArgument is marked as object?.
I started on that, but with that project everything could be null or is set indirectly. So that will take a while.

Because the original Mono.Cecil code is not mine and is quite complex, I only created a subset of it and limited the changes I made. Most recently, discovered it returns null when asked to create an AssemblyDefinition for an unmanaged assembly. I handled that in the caller rather than modifying it.

If we wanted to modify it further, then I think we should probably first upgrade nunit-console to use the latest version, which is 3.0.3.

CharliePoole · 2024-12-30T19:05:58Z

In ExtensionManager.FindExtensionsInAssembly has a local assemblyTargetFramework which is ever only set to null, but its properties are used in exception and log messages. Note this is also the case in the main branch. I see several changes to this code part, but not sure the intention. There was an TargetFramework property on IExtensionAssembly but that is commented out as the definition of RuntimeFramework is in nunit.engine and hence not available in nunit.engine.api.
The result of this is that ExtensionNode.TargetFramework is always null.

Actually, this is sort of a good thing. Since the build works with this field always set to null, I believe I can go ahead and remove the field entirely. I'm not sure how this partial change got into the code but I'd like to complete it.

As with some other comments, I'll ask whether you want to do it here or have a separate issue and PR. The latter is my usual preference but may cause merge conflicts unless we wait till after you're done.

CharliePoole · 2024-12-30T19:15:00Z

ExtensionNode.Path says it must be unique, but as it is not set by a factory it can be null for all of them or multiple instances can have the same path. There is no code to check for uniqueness. The only way to guarantee that is by creating a factory that has a dictionary of all paths and make an internal constructor that also expects the path and remove the public setter. But as the class is public that would be breaking.

I think this is checked in ExtensionManager, but I'm not sure. It's not the construction that causes a problem but the use of a duplicate path in the manager. I'll have to check further.

CharliePoole · 2024-12-30T20:14:26Z

This was taking too long, so I've done all the rest of your comments in one piece.

I agree with you on all of those below. I have only commented when I had something to add.

I'm seeing this is a much bigger job than I had thought it would be. I think it would help me if we could have a chat. Are you up for that? If so, let's pick a date and time. I'm at https://whereby.com/charlie-poole but not always paying attention to it.

8. Several places open a RegistryKey without disposing it.

9. The `TestAgentRunner` checks there is a single package, but in the `Load` it does another selection on !subPackage() and a `for` loop. If the runner only supports a single package then the loop in `Load` is not needed.

10. `TestAgentRunner.TestDomain` is a property used in the base class, but expected to be set in a derived class. Ideally this should be passed in a constructor as we now have no way of guaranteeing it is set by a (3rd party) derived class.
    I added a `Guard.OperationValid` to the `Load` method.

At present, there's no way for anyone to inject a third-party agent runner, although I suspect there will be in future.

11. `InternalTraceWriter.Write(string? value)` calls `base.Write` instead of `_writer.Write`. Eventually that will call the `Write(char)` overload but the same could be said for the `WriteLine` overload. Seems inconsistent.

12. In `TestAgentRemotingTransport` `ITestEngineRunner` implemenation the user must call `CreateRunner` before calling any of the interface methods. There is no way to force that is actually done unless we return a wrapper class with a non-null runner. Again a breaking change. For now I added a (new) ShouldNotBeNull Guard.
    Another thing is that if users call `CreateRunner` twice we might have a "leak". I added a conditional Dispose before creating a new one.

Is this really a problem? The "user" in this case is another internal class.

13. `TcpChannelUtils.CreateTcpChannel` had a _limit_ parameter who's usage is commented out. Should it be removed or re-instated?

Remove

14. `NotRunnableFrameworkDriver` relies on derived classes to set _runstate_, _result_ and _label_. A better solution would have been virtual properties for these field with implementations in the derived classes. But again a breaking change.

This is the time for breaking changes, before there is a release of 4.0.

15. Same for `NotRunnableTestRunner` but here the derived class doesn't set them.

16. `IFrameworkDriver` depends on users calling `Load` first. A design where `Load` return a callable object would fit better with nullability.

I agree. I'm afraid there are a number of things that were OK before and not so OK now.

17. `NUnit3FrameworkDriver` passes in a `CallbackHandler` to the actions and depends on that method calling this once. Could these method not return a `string` instead of calling a callback? We have now no guarantee that the api will call that handler. Whilst the `NUnitNetCore31Driver` calls the method that return values. Why the difference? Can the framework driver not be the same?

The old API is how lots of folks were doing test frameworks back at the start of this century, and we did it as well. At a certain point, Rob developed a new API for use with .NET Core but he kept the old API as well. I think that was a mistake, since the new API is much better. Based on inspection, I think the new API could be used exclusively but I haven't tested it. This seems like a big enough change that it should be done separately though. What do you think?

18. `Path.GetDirectoryName` is very annoying, so is DirectoryInfo.Parent.

Annoying? Not sure which part of the code you mean.

19. nunit.engine.core uses space and nunit.engine.core.test uses tabs.

Not sure how that happened. We try to use spaces everywhere.

20. There are various places that test a variable for `null` then call `log.Error` but then continue accessing that `null` variable.
    I cannot find that `Logger.Error` aborts.

if (method == null)
    log.Error($"Method {methodName} was not found in {_frameworkControllerType.Name}");
log.Debug($"Executing {method.DeclaringType}.{method.Name}");

Yes, the notion at the time was that we log the error and then let the null cause an error. We usually did that for "this should never happen" conditions. I don't claim that it's good, just how it was done. :-)

manfred-brands · 2024-12-31T09:58:12Z

As with some other comments, I'll ask whether you want to do it here or have a separate issue and PR. The latter is my usual preference but may cause merge conflicts unless we wait till after you're done.

For traceability changes that are not directly related to nullable should be done as a minimum as separate commits but better with separate issues and PRs.

We can sort out merging manually.

CharliePoole · 2024-12-31T10:20:48Z

Good. That's how I prefer to do it. Look for new updates you may want to rebase to.

CharliePoole added the Build label Dec 15, 2024

CharliePoole added this to the 4.0 milestone Dec 15, 2024

CharliePoole mentioned this issue Dec 15, 2024

Update Language Level to C# 12 #1540

Closed

CharliePoole added High Priority Blocked and removed Blocked labels Dec 15, 2024

CharliePoole modified the milestones: 4.0, 4.0.0-beta.1 Dec 29, 2024

CharliePoole assigned manfred-brands Dec 29, 2024

CharliePoole mentioned this issue Dec 29, 2024

Isolate agents from the engine itself #1578

Open

This was referenced Dec 30, 2024

WIP: Version4 nullable #1579

Closed

WIP: Enable nullable #1580

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V4 Codebase Should be Null-Enabled #1539

V4 Codebase Should be Null-Enabled #1539

CharliePoole commented Dec 15, 2024

manfred-brands commented Dec 29, 2024

mikkelbu commented Dec 29, 2024

CharliePoole commented Dec 29, 2024 •

edited

Loading

manfred-brands commented Dec 30, 2024

CharliePoole commented Dec 30, 2024 via email

CharliePoole commented Dec 30, 2024

CharliePoole commented Dec 30, 2024

CharliePoole commented Dec 30, 2024

CharliePoole commented Dec 30, 2024

CharliePoole commented Dec 30, 2024

CharliePoole commented Dec 30, 2024

manfred-brands commented Dec 31, 2024

CharliePoole commented Dec 31, 2024

V4 Codebase Should be Null-Enabled #1539

V4 Codebase Should be Null-Enabled #1539

Comments

CharliePoole commented Dec 15, 2024

manfred-brands commented Dec 29, 2024

mikkelbu commented Dec 29, 2024

CharliePoole commented Dec 29, 2024 • edited Loading

Current Dependencies

Planned Dependencies

Notes

manfred-brands commented Dec 30, 2024

CharliePoole commented Dec 30, 2024 via email

CharliePoole commented Dec 30, 2024

CharliePoole commented Dec 30, 2024

CharliePoole commented Dec 30, 2024

CharliePoole commented Dec 30, 2024

CharliePoole commented Dec 30, 2024

CharliePoole commented Dec 30, 2024

manfred-brands commented Dec 31, 2024

CharliePoole commented Dec 31, 2024

CharliePoole commented Dec 29, 2024 •

edited

Loading