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

Add support for running/debugging NUnit tests #683

Merged
merged 4 commits into from
Dec 6, 2016
Merged

Add support for running/debugging NUnit tests #683

merged 4 commits into from
Dec 6, 2016

Conversation

JPAhnen
Copy link
Contributor

@JPAhnen JPAhnen commented Nov 26, 2016

Detect NUnit tests and return an appropriate feature name. When a test
is executed, the test framework name must be set (xunit or nunit). If no
test framework is set, xunit is used (for backwards compatibility).

Detect NUnit tests and return an appropriate feature name. When a test
is executed, the test framework name must be set (xunit or nunit). If no
test framework is set, xunit is used (for backwards compatibility).
@filipw
Copy link
Member

filipw commented Nov 28, 2016

This will also need VS Code change as it currently explicitly searches for XunitTestMethod only.

Generally I think any 3rd party framework specific things shouldn't be embedded into OmniSharp, but at the same xunit/nunit are de facto standard for .NET testing so it's not a problem here imho. 👍

@DustinCampbell
Copy link
Contributor

The VS Code side change is here: dotnet/vscode-csharp#996.

@filipw
Copy link
Member

filipw commented Nov 28, 2016

great!

Copy link
Contributor

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

Minor nits only

private static bool IsTestMethod(MethodDeclarationSyntax node,
SemanticModel sematicModel)
private bool IsTestMethod(MethodDeclarationSyntax node,
SemanticModel sematicModel, Func<ITypeSymbol, bool> predicate)
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting

bool isTestMethod = false;
string featureName = null;

if(IsTestMethod(method, semanticModel, IsDerivedFromFactAttribute))
Copy link
Contributor

Choose a reason for hiding this comment

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

spacing between 'if' and '('

}

symbol = symbol.BaseType;
} while (symbol.Name != "Object");
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting nit: while should go on separate line.

Copy link
Contributor

Choose a reason for hiding this comment

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

More accurate way of doing this:

while (symbol.SpecialType != SpecialType.System_Object);

@JPAhnen
Copy link
Contributor Author

JPAhnen commented Nov 28, 2016

The changes in the VS Code plugin and in Omnisharp-Roslyn work independently. The new Omnisharp-Roslyn will work with the old plugin, and the new plugin will work with the "old" Omnisharp-Roslyn. In both cases xUnit testing still works.

@DustinCampbell
Copy link
Contributor

I hadn't looked carefully, but yes, it does appear that it'll still work. I had assumed that the change to the protocol would break things, but it looks OK. That said, I'll wait to pull the change into VS Code's master until I've got an OmniSharp build to support it. I want to be sure the feature lights up when I take the PR.

@david-driscoll
Copy link
Member

Looks good to me, @DustinCampbell we still waiting on the minor nits?

@DustinCampbell
Copy link
Contributor

Sorry for taking so long to get back to this. Nope, I wasn't waiting on the nits.

@DustinCampbell
Copy link
Contributor

Updated the base branch. I'll go ahead and merge this once the CI build finishes.

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.

4 participants