Skip to content
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

Combine two NUnit3 Drivers #1597

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

Combine two NUnit3 Drivers #1597

wants to merge 8 commits into from

Conversation

CharliePoole
Copy link
Member

Fixes #1596

@CharliePoole
Copy link
Member Author

This is still in progress, but comments are helpful anyway, especially about direction I'm taking.

The first two commits result in a combined driver using conditional compilation. The driver has the working name of NUnitCombinedFrameworkDriver but I hope to find a better one, possibly just NUnitFrameworkDriver since it works for both v3 and v4.

Currently the conditional code is scattered around the file, but that will change in the next commit.

@CharliePoole
Copy link
Member Author

The third commit wraps the two APIs in individual classes and implements a compile-time strategy pattern using API2009 for .NET Framework and API2018 for .NET Core.

@CharliePoole
Copy link
Member Author

Fourth commit does some refactoring and removes the two classes being replaced. Tne new driver is called NUnitFrameworkDriver, eliminating the '3' in the name.

At this point, the choice of interface is still determined at compile time. I've tried two different approaches for making the decision at runtime and neither worked. See #1596 for discussion of problems.

@CharliePoole
Copy link
Member Author

The fifth and sixth commits don't advance towards the goal, but perform refactoring to make further progress easier.

The various Api classes moved into separate files in the fifth commit.

In the sixth, I remove extraneous code from TestAgentRunner, which is based on the possibility of having multiple drivers execute in the same appdomain. This has been unused for some time since there is a one-to-one mapping across test assemblies, agents and domains.

@CharliePoole
Copy link
Member Author

CharliePoole commented Jan 16, 2025

The seventh commit finally reaches the goal of running .NET Framework tests using the 2018 API. It's a pretty big change, doubling the number of files affected and, unfortunately, making a number of unrelated changes that were needed.

A key problem was that a newly created driver didn't have an ID set in it's constructor. Rather, it was set later after construction - usually immediately after. There were various alternative ways to handle this, all a bit disruptive. I chose to modify the constructor for the driver as well as the IDriverFactory interface. This allows the constructor to complete initialization of the ID, which is a must-not-be-null property.

Remaining to do:

  • Parameterize the driver tests so that both APIs are tested under .NET Framework
  • Select the exact version cutoff for using each API. My first guess would be 3.10+ for Api2018, but I'll try a range of them.

@manfred-brands This is still WIP but please take a look. Let me know if you'd rather see this as several PRs. Now that I know it works, I can easily create a new PR for the first six commits and make the rest of it a second PR.

UPDATE: Failing CI due to .NET 7.0 not installed. For now, I made sure it's installed. I'll remove it and add .NET 9.0 in a separate issue and PR.

@CharliePoole CharliePoole changed the title WIP: Combine two NUnit3 Drivers Combine two NUnit3 Drivers Jan 16, 2025
@CharliePoole
Copy link
Member Author

The eighth commit refactors the tests in order to ensure that both APIs we support under .NET Framework are tested. An internal constructor is added to NUnitFrameworkDriver to support the tests.

The driver now switches between the 2009 and the 2018 APIs based on the version of NUnit in use. I was happily surprised to discover that the new API works for versions 3.2 and higher. Very rudimentary package tests have been added to ensure that the switching takes place correctly, but for most cases we now use the newer API. If problems arise, it would be possible to add some sort of switch (e.g. an environment variable) to force use of the older API. I don't see much point in exposing it as a command-line option.

This is now ready for review. @manfred-brands See my earlier comment about splitting it up if it seems desirable.

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.

1 participant