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.

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

Support to run MSTest based tests. #856

Merged
merged 5 commits into from
May 16, 2017
Merged

Support to run MSTest based tests. #856

merged 5 commits into from
May 16, 2017

Conversation

AbhitejJohn
Copy link
Contributor

@AbhitejJohn AbhitejJohn commented May 13, 2017 •
edited
Loading

Accompanying PR : dotnet/vscode-csharp#1478

@AbhitejJohn AbhitejJohn mentioned this pull request May 13, 2017
Merged
@AbhitejJohn
Copy link
Contributor Author

Tagging @DustinCampbell for a review.

david-driscoll
david-driscoll approved these changes May 14, 2017
View reviewed changes
Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

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

LGTM. Still going to wait for dustin if he has any comments, but looks pretty straight forward.

@willl
Copy link
Member

willl commented May 15, 2017

Should unit tests/test projects be added as part of this @david-driscoll?

They exist for xUnit and NUnit.

DustinCampbell
DustinCampbell requested changes May 15, 2017
View reviewed changes
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.

Tests should be added for this change. It should be pretty easy since there are already tests for xUnit and NUnit to base them on.

filipw
filipw reviewed May 15, 2017
View reviewed changes
src/OmniSharp.DotNetTest/TestFrameworks/MSTestFramework.cs Outdated
protected override bool IsTestAttributeName(string typeName)
{
return typeName == "Microsoft.VisualStudio.TestTools.UnitTesting.TestMethodAttribute"
|| typeName == "Microsoft.VisualStudio.TestTools.UnitTesting.DataTestMethodAttribute";
Copy link
Member

Choose a reason for hiding this comment

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

DataTestMethodAttribute is not needed here, because it inherits from TestMethodAttribute.
The code in OmniSharp checks the whole inheritance tree for the attributes.

Copy link
Member

Choose a reason for hiding this comment

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

this is similar to the xunit code in OmniSharp not checking for TheoryAttribute -> because it inherits from FactAttribute

@DustinCampbell
Copy link
Contributor

DustinCampbell commented May 15, 2017 •
edited
Loading

To add tests:

  1. Add a test project it test-assets/test-projects similar to XunitTestProject.
  2. Add the test project to our build plan here. This will ensure that the build script restores and builds the test project.
  3. Add tests to RunTestFacts.cs and GetTestStartInfoFacts.cs.

@AbhitejJohn
Copy link
Contributor Author

Thanks @DustinCampbell . Will update the PR soon. Would we need one for the legacy scenarios as well? I see a LegacyXunitTestProject & LegacyNunitTestProject .

@DustinCampbell
Copy link
Contributor

There's no need to add a legacy test project. Those are for older project.json. Those tests are just to ensure we don't break that older support. We never supported MSTest for project.json .NET Core projects (was it even possible?)

@AbhitejJohn
Copy link
Contributor Author

We never supported MSTest for project.json .NET Core projects (was it even possible?)

MSTest tests did work with project.json. There was a dotnet-test-mstest that drove those scenarios which leads me to guess that there are more changes required to enable that support in.

@DustinCampbell
Copy link
Contributor

If that's the case, this change will likely make MS Test work for project.json projects. It might be worth adding some legacy tests as well. There are LegacyRunTestFacts and LegacyGetStartInfoFacts.

@DustinCampbell DustinCampbell mentioned this pull request May 15, 2017
Closed
@AbhitejJohn
Tests and PR comments addressed.
3722af1 
The legacy tests however are not running as expected and hence are commented for now. In the process of figuring out why this bombs.
@AbhitejJohn
Copy link
Contributor Author

@dotnet-bot test this please.

DustinCampbell
DustinCampbell approved these changes May 15, 2017
View reviewed changes
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.

Changes look good!

@DustinCampbell
Copy link
Contributor

@AbhitejJohn: Note that this repo doesn't use the @dotnet-bot build/test infrastructure.

@AbhitejJohn
Copy link
Contributor Author

@DustinCampbell : Oh ok. It looks like we have a problem with the Legacy workflow though - When we run the test we pass in a commandline similar to dotnet.exe ..... --wait-command --test Main.Test.MainTest.Test. dotnet-test-mstest however expects that when a --wait-command is passed in, the tests to be run would be sent via a RunTestsMessage message as this doc details.

--wait-command: Indicates that the runner should wait to receive commands through the TCP channel instead of running tests right away. We use this to get around the command line size limit.

This step however seems to be missing in the vs code implementation which expects the adapter to pick up the tests from the commandline arguments itself. That seems to be confusing dotnet-test-mstest which defaults to running all the tests in the source instead. Should we change this workflow to reflect the above instead - either remove --wait-command or send out the filtered tests via a message to the adapters? The VS 2015 workflow follows this sequence of steps and things do work out well there.

@DustinCampbell
Copy link
Contributor

@AbhitejJohn: Yeah, I don't think the project.json support was ever truly completed. It was gotten to a point of "good enough" and somewhat abandoned when the developer who worked on it moved on. While adding the new vstest support, I tried to just preserve the legacy support since there will be fewer and fewer people using it.

I'd recommend removing --wait-command. Note that the legacy support already trims off several other arguments: https://github.com/OmniSharp/omnisharp-roslyn/blob/dev/src/OmniSharp.DotNetTest/Legacy/LegacyTestManager.cs#L160-L164.

@AbhitejJohn
Copy link
Contributor Author

@DustinCampbell : Fixed in the latest update.

@DustinCampbell
Copy link
Contributor

Thanks @AbhitejJohn! Looks good! Do you have any other changes? If not, I'll merge as soon as CI completes.

@AbhitejJohn
Copy link
Contributor Author

@DustinCampbell : No, I'm done.

@DustinCampbell DustinCampbell merged commit 0b7b10e into OmniSharp:dev May 16, 2017
@AbhitejJohn AbhitejJohn deleted the mstestsupport branch May 17, 2017 08:18
@DustinCampbell DustinCampbell mentioned this pull request Aug 9, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filipw filipw filipw left review comments

@david-driscoll david-driscoll david-driscoll approved these changes

@DustinCampbell DustinCampbell DustinCampbell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AbhitejJohn @willl @DustinCampbell @david-driscoll @filipw