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

Blame for vstest #915

Merged
merged 10 commits into from
Jul 14, 2017
Merged

Blame for vstest #915

merged 10 commits into from
Jul 14, 2017

Conversation

vagisha-nidhi
Copy link
Contributor

This feature gets the faulty test case name in event of a test host crash of reason unknown.
There are various scenarios in which the test host crashes due to a faulty test case and the reason cannot be diagnosed.
This pull request gets the name of the test case which caused the crash and displays it on the console.
To enable the blame mode, use --Blame|/Blame

The usage is as follows -

'''
"vstest.console.exe" /Testadapterpath:"C:\TestProject\TestProject\bin\Debug\net46" C:\path\to\your\assembly.dll /Blame
'''
crsh

This feature gets the faulty test case name in event of a test host crash of reason unknown.
There are various scenarios in which the test host crashes due to a faulty test case and the reason cannot be diagnosed.
This pull request gets the name of the test case which caused the crash and displays it on the console.
To enable the blame mode, use --Blame|/Blame
@msftclas
Copy link

msftclas commented Jul 6, 2017

@vagisha-nidhi,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

Copy link
Contributor

@codito codito left a comment

Choose a reason for hiding this comment

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

Suggest a few changes.

@@ -266,6 +266,13 @@ function Publish-Package
Write-Verbose "Move-Item $coreCLR20PackageDir\$file $coreCLRExtensionsDir -Force"
Move-Item $coreCLR20PackageDir\$file $coreCLRExtensionsDir -Force
}

# Publish Datacollector
Copy link
Contributor

Choose a reason for hiding this comment

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

Require change in build.sh

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this assembly be added for signing?

@@ -0,0 +1,147 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is there a naming convention for extensions? TrxLogger is named as Microsoft.TestPlatform.Extensions.TrxLogger, should we follow M.TP.E.Blame?

@@ -0,0 +1,58 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Enable stylecop.

</PropertyGroup>
<PropertyGroup>
<AssemblyName>Microsoft.TestPlatform.BlameDataCollector</AssemblyName>
<TargetFrameworks>netcoreapp1.0;netcoreapp2.0;net46</TargetFrameworks>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not build for netstandard only?

<PropertyGroup>
<AssemblyName>Microsoft.TestPlatform.BlameDataCollector</AssemblyName>
<TargetFrameworks>netcoreapp1.0;netcoreapp2.0;net46</TargetFrameworks>
<WarningsAsErrors>true</WarningsAsErrors>
Copy link
Contributor

Choose a reason for hiding this comment

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

Already used in testplatform.settings.targets?

ValidateArg.NotNull<object>(sender, "sender");
ValidateArg.NotNull<TestRunCompleteEventArgs>(e, "e");

if (!e.IsAborted) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please wrap if clauses in parenthesis.

if (attachmentSet.DisplayName.Equals(Constants.BlameDataCollectorName))
{
var filepath = attachmentSet.Attachments[0].Uri.LocalPath;
List<TestCase> testCaseList = this.blameReaderWriter.ReadTestSequence(filepath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use Enumerable.LastOrDefault?

/// <summary>
/// Test run Abort
/// </summary>
public const string TestRunAbortReason = "The test run was aborted because the host process crashed unexpectedly while executing test ";
Copy link
Contributor

Choose a reason for hiding this comment

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

Not used?


#region Test Assemblies

[assembly: InternalsVisibleTo("Microsoft.TestPlatform.BlameDataCollector.UnitTests, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this required.

}

[TestMethod]
public void TriggerTestCaseStartedHandlerShouldIncreaseTestStartCount()
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is testing an internal implementation detail. We should care about the user behavior.

// In case of crash testStartCount will be greater than testEndCount and we need to send write the sequence
if (this.testStartCount > this.testEndCount)
{
var filepath = Path.Combine(AppContext.BaseDirectory, Constants.AttachmentFileName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check filepath

/// <summary>
/// Uri used to uniquely identify the Blame logger.
/// </summary>
public const string ExtensionUri = "logger://Microsoft/TestPlatform/Extensions/Blame";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Version

if (testCaseList.Count > 0)
{
var testcase = testCaseList[testCaseList.Count - 1];
return testcase.FullyQualifiedName;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

LastOrDefault

<ItemGroup>
<ProjectReference Include="..\Microsoft.TestPlatform.ObjectModel\Microsoft.TestPlatform.ObjectModel.csproj" />
<ProjectReference Include="..\..\src\Microsoft.TestPlatform.CoreUtilities\Microsoft.TestPlatform.CoreUtilities.csproj" />
</ItemGroup>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enable stylecop

@@ -129,6 +129,7 @@
<!-- NetCoreExtensions -->
<CoreAssembliesToSign Include="$(ArtifactsCoreDirectory)Extensions\Microsoft.VisualStudio.TestPlatform.Extensions.Trx.TestLogger.dll" />
<CoreAssembliesToSign Include="$(ArtifactsCoreDirectory)Extensions\Microsoft.TestPlatform.TestHostRuntimeProvider.dll" />
<CoreAssembliesToSign Include="$(ArtifactsCoreDirectory)Extensions\Microsoft.TestPlatform.Extensions.BlameDataCollector.dll" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add for net46

@codito codito merged commit c62f4d3 into microsoft:master Jul 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants