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

Failed to run test because null with nested test classes. #1151

Closed
shaunluttin opened this issue Jan 26, 2017 · 9 comments
Closed

Failed to run test because null with nested test classes. #1151

shaunluttin opened this issue Jan 26, 2017 · 9 comments
Assignees
Milestone

Comments

@shaunluttin
Copy link

Environment data

dotnet --info output: 1.0.0-preview2-003133
VS Code version: 1.9.0-insider
C# Extension version: 1.6.2

Steps to reproduce

For us the problem was nested classes. OmniSharp cannot handle this, when we click run test.

    public class SomeClass
    {
        public class SomeMethod
        {
            // Trying to run this test causes
            // "Failed to run test because null"
            [Fact]
            public void SomeTest()
            {
    
            }
        }
    }

Expected behavior

The test runs and the output says "Test Passed".

Actual behavior

Failed to run test because null

capture

@gregg-miskelly
Copy link
Contributor

Thanks. This looks like a dupe of #1100

@shaunluttin
Copy link
Author

shaunluttin commented Jan 26, 2017

@gregg-miskelly It is not necessarily a dupe (depending on how we define dupe), because there are multiple reasons why Omnisharp might give that error. See also: http://stackoverflow.com/q/40858172/1108891

@gregg-miskelly
Copy link
Contributor

Okay, I will reopen for now then.

@DustinCampbell
Copy link
Member

Yeah, this has a different error. However, the "dupe" is from several months ago, so it might just be that behavior has changed since then. I suspect we started hitting null with this change: OmniSharp/omnisharp-roslyn#683

FWIW, this will require a fix in omnisharp-roslyn. Looking at the code in https://github.com/OmniSharp/omnisharp-roslyn/blob/master/src/OmniSharp.DotNetTest/Helpers/TestFeaturesDiscover.cs, I can see how it might fail to locate tests in nested classes. I'll take a look at this shortly.

@DustinCampbell DustinCampbell added this to the 1.7 milestone Jan 26, 2017
@DustinCampbell
Copy link
Member

shortly == tomorrow. 😄

@DustinCampbell DustinCampbell self-assigned this Jan 26, 2017
@DustinCampbell
Copy link
Member

Note: There's also #743

@DustinCampbell
Copy link
Member

Looks like the problem is that there's some hacky code in omnisharp-roslyn that searches specifically for the presence of 'project.json'. https://github.com/OmniSharp/omnisharp-roslyn/blob/dev/src/OmniSharp.DotNetTest/Helpers/ProjectPathResolver.cs#L11

@DustinCampbell
Copy link
Member

OK. I see why the problem is specific to tests in nested classes. The offending line of code is here: https://github.com/OmniSharp/omnisharp-roslyn/blob/dev/src/OmniSharp.DotNetTest/Helpers/TestFeaturesDiscover.cs#L41.

Essentially, the code expects Roslyn's ISymbol.ToDisplayName() to return a name that can be used with 'dotnet test'. However, that method returns strings for display. Between a class and a nested class, it uses a '.' separator. However, 'dotnet test' expects a metadata name when the class and nested class are separated by a '+'. I had to write code that produced a metadata name once before -- for Code Model in VS (https://github.com/dotnet/roslyn/blob/master/src/VisualStudio/Core/Impl/CodeModel/MetadataNameHelpers.cs). Looks like I might have to do that again. 😄

@DustinCampbell
Copy link
Member

OK. I've got a fix for this in omnisharp-roslyn.

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

No branches or pull requests

3 participants