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 ability to use custom runsettings for tests #1710

Merged
merged 5 commits into from Mar 3, 2020

Conversation

@Samirat
Copy link
Contributor

Samirat commented Feb 12, 2020

Currently, the RunSettings are sent to Omnisharp as a a path, and then loaded. Alternatively, we could load the runsettings in the extension, and then send XML with the request.

Related PR to omnisharp-vscode: OmniSharp/omnisharp-vscode#3573

@@ -101,18 +106,18 @@ private static void VerifyTestFramework(string testFrameworkName)
}
}

public override GetTestStartInfoResponse GetTestStartInfo(string methodName, string testFrameworkName, string targetFrameworkVersion)
public override GetTestStartInfoResponse GetTestStartInfo(string methodName, string runSettings, string testFrameworkName, string targetFrameworkVersion)

This comment has been minimized.

Copy link
@filipw

filipw Mar 2, 2020

Member

normally we do not break the public APIs but this particular package is no published on nuget, so it's OK

This comment has been minimized.

Copy link
@Samirat

Samirat Mar 2, 2020

Author Contributor

Even though these functions are public, TestManager and VSTestManager are actually internal classes, so there probably isn't any worry.

This comment has been minimized.

Copy link
@Samirat

Samirat Mar 2, 2020

Author Contributor

I was more concerned about the changes to the over-the-wire interface. At least the stdio version of the server seems to treat arguments as optional, in which case I think everything remains compatible.

{
if (runSettingsPath != null)
{
return File.ReadAllText(runSettingsPath);

This comment has been minimized.

Copy link
@filipw

filipw Mar 2, 2020

Member

you should check if the file exists, if not, I'd fallback to defaults to not crash?

This comment has been minimized.

Copy link
@Samirat

Samirat Mar 2, 2020

Author Contributor

Added an error message if runsettings load fails, and it will continue with default settings.

[Fact]
public async Task RunMSTestWithoutRunSettings()
{
await RunDotNetTestAsync(

This comment has been minimized.

Copy link
@filipw

filipw Mar 2, 2020

Member

if this is supposed to fail, could you assert on some failure conditions to distinguish a "failure by design" from "failure because something got broken"?

This comment has been minimized.

Copy link
@Samirat

Samirat Mar 2, 2020

Author Contributor

Added some additional assertions.

@filipw

This comment has been minimized.

Copy link
Member

filipw commented Mar 2, 2020

thanks a lot for the PR - looks very good, just left some small comments

@filipw
filipw approved these changes Mar 3, 2020
Copy link
Member

filipw left a comment

LGTM thanks!

@filipw

This comment has been minimized.

Copy link
Member

filipw commented Mar 3, 2020

@JoeRobich @mholo65 @david-driscoll any objections on this?

@filipw filipw merged commit b60f3e4 into OmniSharp:master Mar 3, 2020
6 checks passed
6 checks passed
OmniSharp.omnisharp-roslyn Build #1.34.14-PullRequest1710.8 succeeded
Details
OmniSharp.omnisharp-roslyn (GitVersion) GitVersion succeeded
Details
OmniSharp.omnisharp-roslyn (Linux) Linux succeeded
Details
OmniSharp.omnisharp-roslyn (Release) Release succeeded
Details
OmniSharp.omnisharp-roslyn (Windows) Windows succeeded
Details
OmniSharp.omnisharp-roslyn (macOS) macOS succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.