Improve crash/hang support in helix #24489
Conversation
ghost
added
the
area-infrastructure
|
cc @javiercn |
| 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)); |
There was a problem hiding this comment.
Wasn't this requirement a point-in-time thing that's since been resolved❔ @MattGal were you involved in the previous conversations around this❔
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Ok cool, I'll try removing the extra copy to the parent
| @@ -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\""; | |||
There was a problem hiding this comment.
Oops, doesn't vstest expect a project or solution file❔ I'm hoping there's no need to change the subcommand.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
thoughts @nohwnd as he's the expert on these new options
|
Yeah should have mentioned this is the result of #23862 which has examples of what hangs and crashes look like now |
There was a problem hiding this comment.
@ViktorHofer you were saying we should use a RunSettings file? @nohwnd do you have any suggestions here? I'm ok with that approach as long as we're not hardcoding physical paths to anything 👍🏾
|
I'm going to do the runtest settings in a separate PR |
|
Followup PR for runsettings here: #24503 |
|
We are dynamically generating the runsettings file as it contains the hard-coded path to our custom dotnet test shared framework and some dynamic code coverage settings. I think that approach is fine as we copy it into the app's output directory. I will share our generated runsettings file later. |
|
Just as a note, this allows us to use VS and Test Explorer without any env var modifications. |
Switch to test to be able to opt into better blame support which improves crash/dump handling, add additional gathering of dumps for upload