Skip to content

Use correct project capability for test projects #41334

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 13, 2020
Merged

Use correct project capability for test projects #41334

merged 2 commits into from
Feb 13, 2020

Conversation

@drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented Jan 31, 2020

This was flagged in microsoft/VSProjectSystem#328, and further investigation suggests the capability should be TestContainer rather than UnitTestContainer.

It may be that this ItemGroup is unneeded altogether. I'm following up with the unit testing team.

@drewnoakes
Use correct project capability for test projects
41dadf1 
This was flagged in microsoft/VSProjectSystem#328, and
further investigation indicates the capability should be `TestContainer` rather
than `UnitTestContainer`.
@drewnoakes drewnoakes mentioned this pull request Jan 31, 2020
Closed
@sharwell
Copy link
Contributor

sharwell commented Jan 31, 2020

This ItemGroup is unnecessary because it's set here:
https://github.com/xunit/visualstudio.xunit/blob/b107063fd90f9c045d3622f954e6d1e1d4c581e1/src/xunit.runner.visualstudio/build/xunit.runner.visualstudio.desktop.props#L24

@drewnoakes drewnoakes marked this pull request as ready for review February 1, 2020 00:47
@drewnoakes drewnoakes requested a review from a team as a code owner February 1, 2020 00:47
@shyamnamboodiripad
Copy link
Contributor

shyamnamboodiripad commented Feb 3, 2020 •
edited
Loading

Tagging @AbhitejJohn who may know some historical context around the right capability name. But "TestContainer" seems to be what we rely on for the test explorer at the moment. See this check.

shyamnamboodiripad
shyamnamboodiripad approved these changes Feb 3, 2020
View reviewed changes
Copy link
Contributor

@shyamnamboodiripad shyamnamboodiripad left a comment

Choose a reason for hiding this comment

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

I think this should work. To test this you could clean out your .vs folder and make sure that tests are still discovered after building a test project in the sln...

@AbhitejJohn
Copy link
Contributor

AbhitejJohn commented Feb 3, 2020

Yup, I was just talking to @jfleisher about this. We just need the TestContainer check. UnitTestContainer capability seems to be a roslyn only thing. I dont know if any other infra tooling depends on that but from a Test Explorer perspective this change LGTM.

@jinujoseph jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Feb 4, 2020
@drewnoakes
Copy link
Member Author

drewnoakes commented Feb 11, 2020

@jasonmalinowski is there any reason I shouldn't merge this?

@jasonmalinowski
Copy link
Member

jasonmalinowski commented Feb 11, 2020 •
edited
Loading

So how confident are we that something else than the Test Explorer don't depend on this flag? Where did it originally come from?

@drewnoakes
Copy link
Member Author

drewnoakes commented Feb 12, 2020

Looks like @sharwell added it in 1d7c848.

I have found no reference to UnitTestContainer capability anywhere, and @jfleisher confirmed that TestContainer is the expected capability, which is added by targets in unit test packages anyway.

@drewnoakes drewnoakes merged commit 3b88f61 into dotnet:master Feb 13, 2020
@drewnoakes drewnoakes deleted the fix-test-container-capability branch February 13, 2020 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasonmalinowski jasonmalinowski jasonmalinowski approved these changes

@shyamnamboodiripad shyamnamboodiripad shyamnamboodiripad approved these changes

@AbhitejJohn AbhitejJohn AbhitejJohn approved these changes

+1 more reviewer

@sharwell sharwell sharwell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jasonmalinowski jasonmalinowski

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@drewnoakes @sharwell @shyamnamboodiripad @AbhitejJohn @jasonmalinowski @jinujoseph