Enable nullable

[manfred-brands]

Progress towards #1539

nunit.engine.api
nunit.engine.core
nunit.engine.core.tests

[CharliePoole]

I cancelled the build that had been running for five hours. It was hung up on the first unit test run.

Are you able to run the build locally?

[CharliePoole]

Given that this is WIP I've merely commented. You've done a lot of work here and there's nothing that doesn't improve the code. Thanks.

I'll next download your branch and see if I can discover what's breaking the CI.

[CharliePoole]

If you make the change suggested for TestEngine Activator, you can delete this method and the constructor that takes a minimum version.

[manfred-brands]

Done, but note that this Exception doesn't seem to be thrown anywhere from this repository,

[CharliePoole]

The tests are apparently in some other branch or have been deleted. I'll try to find out.

[manfred-brands]

I cancelled the build that had been running for five hours. It was hung up on the first unit test run.

Are you able to run the build locally?

I cannot run tests from VS making debugging harder.
I resorted to adding Debugger.Lauch() calls. Is there a better way?

When running dotnet cake --target=TestNetCorePackage --configuration=Debug it fails finding the nunit.exe to run.
Those pre-existing targets don't seem to link to the recipe as even after calling --target=Build it can't find the executable.

dotnet cake --target=Test runs and fails on InternalTrace. The ExtensionManager calls GetLogger, but the _traceWriter field has not been set because nothing called InternalTrace.Initialize, my check on null here highlighting this issue. Any actual logs written would cause a NullReferenceException. I have changed to default to Console.Out. This doesn't affect any code actually calling initialize, but code that doesn't will output to the Console.

After that, the net462 tests pass, but the netcore tests hang.
An exception is caught and translated into an NUnitEngineException and CurrentResult.RecordException called.
But as this is called from a Load there might be no context.
The exception is that DependencyContext.Load returns null. As the result is used unconditionally in TryResolve, I had added a Guard to assert it wouldn't.
Not sure what that means for RuntimeLibrariesStrategy? Should I check in TryResolve and return false if _dependencyContext is null?

That got me a bit further, until a log.Debug call causing a NRE.
The situation is that some class gets a Logger BEFORE some other part of the code calls InternalTrace.Initialize, invalidating the original logger.
I changed the design so that the logger gets the current writer instead of the writer at time of creation.

[CharliePoole]

Suggestion about the build... verify that you can run it on main branch to see if the problems arise only in your modified branch. Normal command I use is build -t Test -t Package.

I'll look your commits in the morning.

It's 2:00am here but I'm temporarily awake. Do you want to chat a bit? I'd like to figure out a good way to have me make changes without causing you enormous conflicts. If you don't see this right away, I'll be back in bed, however. :-)

[CharliePoole]

The targets in build.cake depend on the older cake files I had you remove. I should have said to remove those as well.

I'm pretty sure that the condition you are seeing with the logs doesn't exist in the original main branch but I'll check in the morning as I'm not really alert enough to do it now.

[manfred-brands]

I'm pretty sure that the condition you are seeing with the logs doesn't exist in the original main branch but I'll check

Set InternalTrace.DefaultTraceLevel to Debug and you should notice it.

[manfred-brands]

I split of the logger changes into #1586 with PR #1588

[manfred-brands]

Remarks on NUnit.Engine:

1. TestSelectionParser has multiple public method which, I think, make no sense as there is no _tokenizer to parse.
   I changed the Parse method to static, which allowed me to set the _tokenizer in the constructor.
   Note that the original class was not thread safe. The TestSelectionParserTests create a single instance for multiple tests.
   If a future runner would run test(cases) in parallel it would not have worked.
   If you rather keep the original, I can create a nested class to get the same behaviour.

2. TestExecutionTask has a disposeRunner flag which calls Dispose on the runner after Execute. Regardless of the flag, the runners are Disposed when the AggregatingTestRunner is disposed. What is the reason for the DisposeRunners flag?

3. The Cyclic Dependency between ServiceandServiceContext` is annoying as it should be declared nullable because if a service is never added to the context it will not be set.

4. ResultService._extensionNodes is only set conditional, but used unconditional. If the ExtensionService is started after the ResultService things will not work. ServiceManager.StartService order depends on the order AddService is called.
   This is mentioned in the comments, but hard to enforce.

5. ServiceContext.GetService some places check for null others don't. Converting the code to throw when a service is not found, causes exceptions in ProcessRunner which requires a TestAgency. I added a registration in the test which has fixed it.

6. RuntimeFramework.CurrentFramework is declared to return an IRuntimeFramework but every test assumes it derived from the actual RuntimeFramework class. Maybe we can remove the interface?

[manfred-brands]

Comments on NUnitConsole

I had a de-ja-vu experience and a hard time finding any differences between nunit4-console and nunit4-netcore-console!
The latter still uses a NUNIT3-CONSOLE string (updated)

The Options class is too big and worse we have two, virtually identical ones.

The class uses the KeyedCollection.Dictionary but that class only creates the dictionary if the number of items exceed the dictionaryCreationThreshold which luckily is 0, but it is not specified in the constructor and hence depending on the baseclass implementation. I added an argument to the constructor.

CommandSet is another one of those cyclic properties that could be null if a Command is not added to a CommandSet.
If command are not useful standalone, they should expect a CommandSet parameter in their constructor.
Something for a separate issue/PR.

[manfred-brands]

@CharliePoole This is ready for review. I have tried to do the minimum of changes to comply with nullability.

I have followed your pattern of referencing a file as link when needed in the console instead of referencing the library it is in.
Items like Guard (with namespace NUnit.Common) is one of those. Maybe it should really be in a separate library shared among the different NUnit repositories.

[CharliePoole]

Remarks on NUnit.Engine:

At this point, I'd like to get all your work merged so we can move on from there.

Let's do issues on some of these, as well as your earlier comments, so they aren't forgotten. I'll go ahead and create themt using your comments as a start. Please chime in on the issues while it's fresh in your mind if you think more info is needed on any of them.

1. `TestSelectionParser` has multiple `public` method which, I think, make no sense as there is no `_tokenizer` to parse.
   I changed the `Parse` method to `static`, which allowed me to set the `_tokenizer` in the constructor.
   Note that the original class was not thread safe. The `TestSelectionParserTests` create a single instance for multiple tests.
   If a future runner would run test(cases) in parallel it would not have worked.
   If you rather keep the original, I can create a nested class to get the same behaviour.

Probably an ISSUE - My thinking here was that our code doesn't exercise the parser in parallel at all. However, there is an existing issue calling for parallel test discovery, which would result in that happening, so this would be good to fix. Of course, if any of our tests won't work if run in parallel, then we probably ought to document that by using `[NonParalellizable].

2. `TestExecutionTask` has a `disposeRunner` flag which calls `Dispose` on the runner after `Execute`. Regardless of the flag, the runners are Disposed when the `AggregatingTestRunner` is disposed. What is the reason for the `DisposeRunners` flag?

Possible ISSUE. I'll review the code.

3. `The Cyclic Dependency between `Service`and`ServiceContext` is annoying as it should be declared _nullable_ because if a service is never added to the context it will not be set.

ISSUE, possibly in combination with item 4.

4. `ResultService._extensionNodes` is only set conditional, but used unconditional. If the `ExtensionService` is started after the `ResultService` things will not work. `ServiceManager.StartService` order depends on the order `AddService` is called.
   This is mentioned in the comments, but hard to enforce.

ISSUE on initialization order. I've considered requiring services to state their dependencies declaratively, which would allow the service manager to discover the order. However, my experience is that almost all users simply use our default initialization..

5. `ServiceContext.GetService` some places check for `null` others don't. Converting the code to `throw` when a service is not found, causes exceptions in `ProcessRunner` which requires a `TestAgency`. I added a registration in the test which has fixed it.

I'll look at the fix and only file an ISSUE if it seems needed.

6. `RuntimeFramework.CurrentFramework` is declared to return an `IRuntimeFramework` but every test assumes it derived from the actual `RuntimeFramework` class. Maybe we can remove the interface?

The removal is coming in #1578 once this is merged.

Comments on NUnitConsole

I had a de-ja-vu experience and a hard time finding any differences between nunit4-console and nunit4-netcore-console! The latter still uses a NUNIT3-CONSOLE string (updated)

Sorry about all these... I had planned to remove the duplicate code before you got to the console. The duplication is a temporary stage on the way to separating the two runners.

The Options class is too big and worse we have two, virtually identical ones.

ISSUE. Ideally they should not have varied. I'll check that out. The class is big because the author intended it to be used in source form. In fact it was once a single file IIRC. The question in my mind is whether to rewrite it entirely at this point.

The class uses the KeyedCollection.Dictionary but that class only creates the dictionary if the number of items exceed the dictionaryCreationThreshold which luckily is 0, but it is not specified in the constructor and hence depending on the baseclass implementation. I added an argument to the constructor.

CommandSet is another one of those cyclic properties that could be null if a Command is not added to a CommandSet. If command are not useful standalone, they should expect a CommandSet parameter in their constructor. Something for a separate issue/PR.

Agreed.

[CharliePoole]

Thanks for doing this. It was quite a lot to go through. I'll merge and follow-up further if I run into any problems trying to rebase my own large change.