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

Create new nunit.extensibility and nunit.extensibility.api assemblies #1612

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

Conversation

CharliePoole
Copy link
Member

Fixes #1049

@CharliePoole
Copy link
Member Author

@manfred-brands As I told you on slack, I'm hoping you'll continue to review these major structural changes I'm making. This is another wide-ranging one and I think it will take some time, but the reviews are really productive. If you're worn out for the moment, I'll ask for volunteers

Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, except for two big issues:

  1. Don't drop nullability. Not even for NUnit tests. The NUnit.Analyzer will fix most errors.
  2. Come up with a better exception than Exception to throw.

build.cake Outdated Show resolved Hide resolved
src/Extensibility/nunit.extensibility/ExtensionManager.cs Outdated Show resolved Hide resolved
src/Extensibility/nunit.extensibility/ExtensionManager.cs Outdated Show resolved Hide resolved
src/Extensibility/nunit.extensibility/ExtensionNode.cs Outdated Show resolved Hide resolved
src/NUnitConsole/nunit4-console/ConsoleRunner.cs Outdated Show resolved Hide resolved
@CharliePoole
Copy link
Member Author

Thanks for reviewing. To your general comments...

  1. You're right... I was experimenting with removing it for tests because I have the impression that it's more trouble than it is worth in tests. That's one thing I'd like to chat about with you, so as to reach a common understanding. However, I did talk with Terje about it and he also favors making tests nullable so that should be the goal.
  2. I'm thinking that as well, I just failed to decide on a name.

@manfred-brands
Copy link
Member

I have the impression that it's more trouble than it is worth in tests.

Why should unit test have a lower standard than the rest of the code?
The nunit.analyzers help a lot, suppressing warnings about uninitialised fields when these are set in Setup methods and when accessing nullable values after testing for Is.Not.Null.

@CharliePoole
Copy link
Member Author

Sorry, but I don't think that not using nullability means "lower standard". We made an effort to keep high standards before it existed. It could mean that maintaining a higher standard becomes easier, which is why I'm working at it now. When it first came out, I didn't believe the hype and since then I've been mostly working alone.

You convinced me to try it, to learn about it. That means you'll see me trying to find out if it's good in various places or not. Be happy for me. :-)

NUnit.Analyzers: I didn't know it did that. That's probably why adding the Nullability.props import without adding the analyzer caused so much trouble. I think this stuff needs to be more centralized, perhaps in Directory.Build.props.

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.

2 participants