-
Notifications
You must be signed in to change notification settings - Fork 152
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
base: version4
Are you sure you want to change the base?
Conversation
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 Currently the conditional code is scattered around the file, but that will change in the next commit. |
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. |
Fourth commit does some refactoring and removes the two classes being replaced. Tne new driver is called 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. |
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. |
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 Remaining to do:
@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. |
1613374
to
b9ce49c
Compare
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 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. |
Fixes #1596