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

Eliminate DomainUsage setting #860

Closed
CharliePoole opened this issue Jan 13, 2021 · 6 comments · Fixed by #1133
Closed

Eliminate DomainUsage setting #860

CharliePoole opened this issue Jan 13, 2021 · 6 comments · Fixed by #1133
Assignees
Milestone

Comments

@CharliePoole
Copy link
Collaborator

In a separate discussion, @ChrisMaddock expressed the desire to eliminate user control of how the runner and engine use AppDomains. I've been through the codebase to look at the impact and I can see it would enormously simplify a lot of code, particularly the test code. This issue is for the purpose of opening discussion on making the change. I'm flagging it as an idea for discussion by the @nunit/engine-team.

Some notes...

  1. We can't eliminate AppDomains themselves for some platforms, they are needed. However, we can make their use an internal implementation detail outside of the control of users.
  2. The command-line --domain option would be dropped.
  3. The DomainUsage enumeration would be dropped.
  4. Both the saved setting and the package setting for DomainUsage would disappear.
  5. All the tests that utilize DomainUsage would be eliminated.
  6. I believe that the nunit project loader would simply ignore the domain setting in .nunit files, but this would have to be tested.
  7. The nunit project loader should probably be modified at some point.
  8. This is a breaking change for the console, engine and nunit project loader extension.

FWIW, I favor doing this as part of a 4.0 release that resolves all the breaking issues we currently have, without necessarily adding a lot of new functionality. The new stuff can come in 4.1, 4.2, etc.

@ChrisMaddock Hope you don't mind that I added a new "breaking" label. 😄

@ChrisMaddock
Copy link
Member

Absolutely agree with this idea - the various domain options add high complexity and complexity for a low use feature.

I presume we would also remove the --domain=None option? Anyone know why that was added, or a sensible use case for it?

@CharliePoole
Copy link
Collaborator Author

I'm now remembering that VS has some way to tell the adapter to run without creating an AppDomain. So this is another one that should involve @OsirisTerje .

@CharliePoole
Copy link
Collaborator Author

I can't remember why it was originally added, but I know I've regretted it for a long time. 😈

I think it has to do with some actions not being permitted in a secondary AppDomain. So some software can't run except in the primary AppDomain. IMO however that's what NUnitLite is for.

@CharliePoole
Copy link
Collaborator Author

@ChrisMaddock Are we ready to move this to a feature and work on it?

@ChrisMaddock
Copy link
Member

I think so. 👍

@CharliePoole
Copy link
Collaborator Author

This was merged to dev-4.0 and closed. I'm reopening it so as to replicate it in main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants