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

[Explicit] tests are executed in Rider/R# #487

Closed
roji opened this issue Mar 16, 2018 · 13 comments
Closed

[Explicit] tests are executed in Rider/R# #487

roji opened this issue Mar 16, 2018 · 13 comments

Comments

@roji
Copy link

@roji roji commented Mar 16, 2018

When running the tests for my entire project in Rider, tests marked with [Explicit] are executed. This issue was investigated by Jetbrains, who concluded that it's an issue on the NUnit side, where there adapter is hard-coded to run explicit ones. The issue also occurs on Visual Studio 15.6.2 with Resharper 2017.3.3.

NUnit: 3.10.1
NUnit3TestAdapter: 3.10.0
Rider: 2017.3.1 (on Linux)
Targets: net45, netcoreapp2.0

Test project csproj:

<Project Sdk="Microsoft.NET.Sdk" ToolsVersion="15.0">
  <PropertyGroup>
    <TargetFrameworks>net451;netcoreapp2.0</TargetFrameworks>
    <DebugType>portable</DebugType>
    <AssemblyOriginatorKeyFile>../../Npgsql.snk</AssemblyOriginatorKeyFile>
    <SignAssembly>true</SignAssembly>
    <PublicSign Condition=" '$(OS)' != 'Windows_NT' ">true</PublicSign>
  </PropertyGroup>
  <ItemGroup>
    <ProjectReference Include="../../src/Npgsql/Npgsql.csproj" />
  </ItemGroup>
  <ItemGroup>
    <PackageReference Include="NUnit" Version="3.10.1" />
    <PackageReference Include="NLog" Version="5.0.0-beta06" />
    <PackageReference Include="Microsoft.CSharp" Version="4.4.1" />
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.5.0" />
    <PackageReference Include="NUnit3TestAdapter" Version="3.10.0" />
  </ItemGroup>
  <ItemGroup Condition=" '$(TargetFramework)' == 'net451' ">
    <Reference Include="System.Transactions" />
  </ItemGroup>
  <ItemGroup>
    <Reference Include="Microsoft.CSharp" />
  </ItemGroup>
</Project>
@roji

This comment has been minimized.

Copy link
Author

@roji roji commented Mar 16, 2018

Note: in the old runner there was nunit/dotnet-test-nunit#25, which led to #47, but that issue is marked as resolved in 3.10, and that doesn't seem to be the case.

@OsirisTerje

This comment has been minimized.

Copy link
Member

@OsirisTerje OsirisTerje commented Mar 16, 2018

The NUnit3TestAdapter is not used by JetBrains, they have their own adapter.

@rprouse

This comment has been minimized.

Copy link
Member

@rprouse rprouse commented Mar 16, 2018

The old test adapter only worked for .NET Core 1.0 and doesn't share code with the other adapters. If JetBrains is somehow using our adapter, it is likely that they are sending us filters for running all tests (as opposed to an empty filter) which explicitly tells us to run all tests which we take to mean including explicit. We'll need more information from the JetBrain's team on how they use our adapter and which filters they pass for Run All Tests.

@roji

This comment has been minimized.

Copy link
Author

@roji roji commented Mar 16, 2018

@rprouse I hope you guys can work with JetBrains to get this resolved...

@jnm2

This comment has been minimized.

Copy link
Contributor

@jnm2 jnm2 commented May 9, 2018

There are two points of entry through which VSTest tells adapters like ours to run tests:

  • An entry point which hands us a parsed filter expression. This is invoked through the CLI.

  • An entry point which hands us a bare list of the previously discovered test cases in the selection to run without telling us how the selection was made. We can't know if you explicitly selected each test case or if it was thrown in under a group header. This is invoked when you run a selection of tests from Test Explorer.

    The only way to truly fix this is if Test Explorer starts actually understanding the concept of explicit selection, or if Test Explorer starts telling us what the UI groupings were which the user actually clicked so that we can deduce per test case whether each one in the selection was explicitly selected.

    Click to see screenshots

    Source-based discovery shows the explicit test only under the Whatever category:

    After building, the Explicit category also shows up (3.10 feature):
    imgpsh_fullsize

    If you right-click the Whatever header and run, we have no way to tell which header you used or whether you selected the child item explicitly:
    imgpsh_fullsize

I'm going to assume Rider is using the latter entry point. If so, the same things that I stated about VS's Test Explorer apply equally to Rider.

@OsirisTerje brought up the idea again that for the latter entry point, we could skip all explicit tests unless the selection comprises only explicit tests. I think that is the absolute best option, given the inherent constraints of the latter entry point.

@jnm2

This comment has been minimized.

Copy link
Contributor

@jnm2 jnm2 commented May 9, 2018

@OsirisTerje can correct me if I'm wrong (I'm the newb), but my understanding has been that we consider the VSTest adapter to be a low-fidelity runner, low-fi out of necessity to adapt to VSTest's current API. Explicitness, warnings, etc got lost or poorly interpreted as a result.

We'd much prefer Rider (and VSTest!) to do a more full-fidelity integration with frameworks like NUnit which have first-class concepts of explicit selection or warnings. Or, at least provide us enough ambient information (e.g. about how the test selection was made) for us to polyfill a full-fidelity experience. This would require a new adapter interface, from what I understand. The current one is too lossy.

This is just dreaming, but wouldn't it be cool if frameworks like NUnit and xUnit and runner interfaces like Test Explorer and VSTest, Rider and ReSharper, NCrunch, and others could compile a list of concepts and unify around a common exchange interface? That would enable high-fidelity interaction information as explicit selections, and high-fidelity results such as warnings.

@estrizhok

This comment has been minimized.

Copy link

@estrizhok estrizhok commented May 15, 2018

@rprouse
In JetBrains we do use your adapter to run tests for .netcore, and yes, you are right that we send a list of tests that needed to be executed to the adapter. That list might include tests marked with [Explicit] attribute, if user had those in their selection. That selection is the reason why send a list populated with specific tests instead of an empty list.
We could have easily excluded tests marked with [Explicit] from the run, if we had such information. Unfortunately, test-cases returned by the adapter on discovery step do not include it. Of course, we could run our own analysis, then merge its result with what test adapter returns, but we would rather use the information provided by the adapter only.
In this regard, could you add an additional custom property to test-cases indicating when they are explicit?

@OsirisTerje

This comment has been minimized.

Copy link
Member

@OsirisTerje OsirisTerje commented May 15, 2018

@estrizhok From version 3.10 of the adapter, all test cases that have an Explicit attribute also has a category added with the same name.
See #47 (comment),

@estrizhok

This comment has been minimized.

Copy link

@estrizhok estrizhok commented May 15, 2018

I have just checked the latest version of adapter (3.10), and I see that Explicit attribute not only passed as a category, but also as a custom property. Excellent!

@OsirisTerje

This comment has been minimized.

Copy link
Member

@OsirisTerje OsirisTerje commented May 15, 2018

Thanks! Superb! Then I hope it works out!
Please give me a hint when it is out, and I'll close this issue then. :-)

@jnm2

This comment has been minimized.

Copy link
Contributor

@jnm2 jnm2 commented May 16, 2018

🎉 This is great news!

Besides the user-visible Explicit category, the hidden bool NUnit.Explicit property which @estrizhok is talking about was added in #471. We had to do this in order to know in the execution phase which ones were truly explicit in the discovery phase.

Now that Rider will be using the NUnit.Explicit bool property as an augmenting side channel, we should add tests which ensure that (for example) this hidden property keeps existing with the same name and type. Even if we make an internal change and end up not using it anymore ourselves, we still want Rider to be able to rely on it.

@estrizhok Will ReSharper gain the same understanding of explicit selections as Rider?

@jnm2

This comment has been minimized.

Copy link
Contributor

@jnm2 jnm2 commented May 23, 2018

Please see #509.

@OsirisTerje OsirisTerje added this to the 3.11 milestone May 23, 2018
@estrizhok

This comment has been minimized.

Copy link

@estrizhok estrizhok commented May 28, 2018

@jnm2 yes, R# and Rider share the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.