Skip to content

Improve crash/hang support in helix #24489

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

Merged
merged 1 commit into from
Aug 1, 2020
Merged

Improve crash/hang support in helix #24489

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion eng/helix/content/RunTests/TestRunner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ public async Task<int> RunTestsAsync()
{
// Timeout test run 5 minutes before the Helix job would timeout
var cts = new CancellationTokenSource(Options.Timeout.Subtract(TimeSpan.FromMinutes(5)));
var commonTestArgs = $"vstest {Options.Target} --logger:xunit --logger:\"console;verbosity=normal\" --blame";
var commonTestArgs = $"test {Options.Target} --logger:xunit --logger:\"console;verbosity=normal\" --blame \"CollectHangDump;TestTimeout=5m\"";
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, doesn't vstest expect a project or solution file❔ I'm hoping there's no need to change the subcommand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note these options aren't particularly well-documented for either dotnet test or dotnet vstest. Might be more correct to use --blame --blame-hang --blame-hang-timeout 5m but it's hard to tell.

Copy link
Member Author

Choose a reason for hiding this comment

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

thoughts @nohwnd as he's the expert on these new options

if (Options.Quarantined)
{
Console.WriteLine("Running quarantined tests.");
Expand Down Expand Up @@ -331,6 +331,22 @@ public void UploadResults()
{
Console.WriteLine("No logs found in artifacts/log");
}
Console.WriteLine($"Copying TestResults/**/*.dmp to {HELIX_WORKITEM_UPLOAD_ROOT}/");
if (Directory.Exists("TestResults"))
{
foreach (var file in Directory.EnumerateFiles("TestResults", "*.dmp", SearchOption.AllDirectories))
{
var fileName = Path.GetFileName(file);
Console.WriteLine($"Copying: {file} to {Path.Combine(HELIX_WORKITEM_UPLOAD_ROOT, fileName)}");
// Need to copy to HELIX_WORKITEM_UPLOAD_ROOT and HELIX_WORKITEM_UPLOAD_ROOT/../ in order for Azure Devops attachments to link properly and for Helix to store the logs
File.Copy(file, Path.Combine(HELIX_WORKITEM_UPLOAD_ROOT, fileName));
File.Copy(file, Path.Combine(HELIX_WORKITEM_UPLOAD_ROOT, "..", fileName));
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't this requirement a point-in-time thing that's since been resolved❔ @MattGal were you involved in the previous conversations around this❔

Copy link
Member

Choose a reason for hiding this comment

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

@dougbu I was OOF Friday but yes, both XUnit and AzDO reporters should now allow this since May or so, via dotnet/arcade#5521 . If the files are small it doesn't really matter, if they're big it could mean a decent perf gain; either way when it's convenient it'd be good to remove this anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok cool, I'll try removing the extra copy to the parent

}
}
else
{
Console.WriteLine("No dmps found in TestResults");
}
}
}
}