Restart Server on Each NetPerf Test Run #4050
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4050 +/- ##
==========================================
- Coverage 87.10% 86.03% -1.07%
==========================================
Files 56 56
Lines 16938 16938
==========================================
- Hits 14753 14573 -180
- Misses 2185 2365 +180 ☔ View full report in Codecov by Sentry. |
| param ($Session) | ||
| if ($isWindows) { | ||
| Invoke-Command -Session $Session -ScriptBlock { | ||
| $DumpDir = "C:/_work/quic/artifacts/crashdumps" |
There was a problem hiding this comment.
Is this common to create on top directory? how about using working directory? or C:/_work/quic is already commonly used for any purpose by msquic perf test?
There was a problem hiding this comment.
This is the "common" working directory for our tests. It's used throughout.
| param ($Session, $OutputDir) | ||
| if ($isWindows) { | ||
| $DumpFiles = Invoke-Command -Session $Session -ScriptBlock { | ||
| Get-ChildItem "C:/_work/quic/artifacts/crashdumps" | Where-Object { $_.Extension -eq ".dmp" } |
There was a problem hiding this comment.
Use globally defined const name?
There was a problem hiding this comment.
Yes, for our perf machines, this is a fixed path.
| $metric = "throughput-download" | ||
| if ($exeArgs.Contains("plat:1")) { | ||
| $metric = "latency" | ||
| } elseif ($exeArgs.Contains("prate:1")) { | ||
| $metric = "hps" |
There was a problem hiding this comment.
How are these "plat" and "prate" associated with each metric name? Looks really high context naming
There was a problem hiding this comment.
As mentioned in the comment above, this stuff is fragile and needs to be improved in future PRs.
| static CXPLAT_THREAD_CALLBACK(WatchdogThreadCallback, Context) { | ||
| auto This = (CxPlatWatchdog*)Context; | ||
| if (!This->ShutdownEvent.WaitTimeout(This->TimeoutMs)) { | ||
| #ifndef _KERNEL_MODE // Not supported in kernel mode | ||
| if (This->WriteToConsole) { | ||
| printf("Error: Watchdog timeout fired!\n"); |
There was a problem hiding this comment.
If error, how about fprintf(stderr, ) ?
There was a problem hiding this comment.
Printing to error stream causes other complexity at the PowerShell level if you decide you want to ignore certain errors (temporarily) like we are doing currently. So I prefer to keep it this way.
Description
Improvements to the NetPerf automation to restart the server for each test, but also includes several other fixes found along the way:
Testing
NetPerf Automation
Documentation
N/A