Add artifact naming service with template support for consistent naming across extensions#7856
Add artifact naming service with template support for consistent naming across extensions#7856Evangelink wants to merge 19 commits intomainfrom
Conversation
Co-authored-by: nohwnd <5735905+nohwnd@users.noreply.github.com>
Co-authored-by: nohwnd <5735905+nohwnd@users.noreply.github.com>
…ssing imports Co-authored-by: nohwnd <5735905+nohwnd@users.noreply.github.com>
Co-authored-by: nohwnd <5735905+nohwnd@users.noreply.github.com>
…ames Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
# Conflicts: # src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpProcessLifetimeHandler.cs # src/Platform/Microsoft.Testing.Platform/Services/ServiceProviderExtensions.cs
…ct-naming-service
- Remove <root> placeholder (fragile directory traversal) - Fix OperatingSystem.IsWindows/IsLinux/IsMacOS for netstandard2.0 compat (use RuntimeInformation) - Fix KeyValuePair deconstruction for netstandard2.0 compat - Fix Guard.NotNullOrEmpty (non-existent API) - Fix legacy %p pattern handling (move to HangDump caller, not the service) - Fix time format to use hyphens instead of colons (filesystem safe) - Remove unnecessary using directives - Simplify GetOperatingSystemName with ternary expression - Simplify TFM detection (remove hard-coded version map) - Fix tests: use Assert.ThrowsExactly, match constructor signature, update assertions
There was a problem hiding this comment.
Pull request overview
This PR introduces an internal IArtifactNamingService to centralize artifact filename/path templating across Microsoft.Testing.Platform extensions, and migrates HangDump to use it (while keeping legacy %p support).
Changes:
- Added
IArtifactNamingService+ArtifactNamingServiceimplementation with placeholder-based template resolution. - Registered the service in
TestHostBuilderand exposed it viaIServiceProviderextension method. - Updated HangDump to use the service and added/updated unit + integration tests plus documentation.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Platform.UnitTests/Services/ArtifactNamingServiceTests.cs | Adds unit tests covering placeholder replacement, casing, unknown placeholders, and error cases. |
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/HangDumpTests.cs | Adds integration coverage for HangDump template-based filenames. |
| src/Platform/Microsoft.Testing.Platform/Services/ServiceProviderExtensions.cs | Adds GetArtifactNamingService() accessor. |
| src/Platform/Microsoft.Testing.Platform/Services/IArtifactNamingService.cs | Introduces the internal artifact naming service contract. |
| src/Platform/Microsoft.Testing.Platform/Services/ArtifactNamingService.cs | Implements regex-based placeholder resolution and default replacement sources. |
| src/Platform/Microsoft.Testing.Platform/Hosts/TestHostBuilder.cs | Registers ArtifactNamingService into the platform service provider. |
| src/Platform/Microsoft.Testing.Platform/Helpers/System/SystemEnvironment.cs | Implements newly added Version property on the environment wrapper. |
| src/Platform/Microsoft.Testing.Platform/Helpers/System/IEnvironment.cs | Adds Version to the environment abstraction. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpProcessLifetimeHandler.cs | Uses IArtifactNamingService for dump filename resolution (then applies legacy %p). |
| src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpExtensions.cs | Wires IArtifactNamingService into HangDumpProcessLifetimeHandler construction. |
| docs/ArtifactNamingService.md | Documents placeholders, custom replacements, and HangDump usage. |
Copilot's findings
- Files reviewed: 11/11 changed files
- Comments generated: 2
|
|
||
| private string GetTargetFrameworkMoniker() | ||
| { | ||
| string frameworkDescription = RuntimeInformation.FrameworkDescription; |
There was a problem hiding this comment.
This is "best-effort" TFM implementation, and doesn't necessarily map to the real/actual TargetFramework used during build time. I'm not sure if this approach is good, or if calling this "tfm" is a correct naming (even if we have that logic already somewhere for display purposes, we are now using it for something more critical than just display).
| /// Service for generating consistent artifact names and paths using template patterns. | ||
| /// Supports placeholders like <pname>, <pid>, <id>, <os>, <asm>, <tfm>, <time>. | ||
| /// </summary> | ||
| internal interface IArtifactNamingService |
There was a problem hiding this comment.
I would personally avoid relying on IVT, and so I wouldn't register it as a service in core MTP at all. Instead, I think we can start small and have that as a simple static helper class that we link in extensions that need it.
| private readonly IClock _clock; | ||
| private readonly IProcessHandler _processHandler; | ||
|
|
||
| private static readonly Regex TemplateFieldRegex = new(@"<([^>]+)>", RegexOptions.Compiled); |
There was a problem hiding this comment.
Isn't this equivalent to <(.+?)>? Is one more efficient than the other? I think <(.+?)> is easier to read.
There was a problem hiding this comment.
@copilot please review equivalence and efficiency
There was a problem hiding this comment.
Both patterns are functionally equivalent for all relevant cases (single-line artifact template strings without > inside placeholder names).
On efficiency: benchmarking 1M iterations with a compiled regex on a typical template like <pname>_<pid>_<id>_hang.dmp shows <(.+?)> is actually marginally faster (~666ms vs ~755ms) because .NET's compiled regex engine has specific optimizations for lazy quantifiers against single-character anchors — no backtracking is needed once the engine sees >.
Changed to <(.+?)> in commit 8307cf1.
| replacements["pname"] = currentProcess.Name; | ||
|
|
||
| // OS info | ||
| replacements["os"] = GetOperatingSystemName(); |
There was a problem hiding this comment.
I'm not sure I see this useful. It's best to start as small as possible IMO to address real pain of users.
The only current real pain I see is making TRX file names flexible per the few reports we got (none of which were related to os).
| replacements["time"] = _clock.UtcNow.ToString("yyyy-MM-ddTHH-mm-ss", CultureInfo.InvariantCulture); | ||
|
|
||
| // Random ID for uniqueness | ||
| replacements["id"] = GenerateShortId(); |
There was a problem hiding this comment.
Per the above high precision time suggestion, I think we won't need an id for uniqueness. It's extremely unlikely the above will produce collisions.
There was a problem hiding this comment.
But if you don't use the time, having the id could be useful
There was a problem hiding this comment.
I don't see why would a user not want to use the time as if it's the "id" for uniqueness.
…lent behavior, slightly faster) Agent-Logs-Url: https://github.com/microsoft/testfx/sessions/20821b8e-ec78-46cf-a7bd-cfbe8387b93c Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
…yy-MM-dd_HH-mm-ss.fffffff Agent-Logs-Url: https://github.com/microsoft/testfx/sessions/c862fbaf-cecd-4c9f-adbc-114c139eff4d Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
…y test Agent-Logs-Url: https://github.com/microsoft/testfx/sessions/c862fbaf-cecd-4c9f-adbc-114c139eff4d Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
|
fyi: In the end I landed on |
Summary
Add
IArtifactNamingService— a reusable service that allows extensions to generate consistent artifact file names using template placeholders.Users can specify templates like:
<pname>_<pid>_<id>_hang.dmp→MyTests_12345_a1b2c3d4_hang.dmp<asm>_<tfm>_<time>.log→MyTests_net9.0_2025-09-22T13-49-34.logAvailable Placeholders
<pname>MyTests<pid>12345<id>a1b2c3d4<os>windows,linux,macos<asm>MyTests<tfm>net9.0<time>2025-09-22T13-49-34Key Design Decisions
IArtifactNamingServiceis internal, registered in the service provider, and available to all extensions%ppattern handled in the HangDump caller viastring.ReplaceHH-mm-ss) instead of colonsChanges
IArtifactNamingServiceinterface andArtifactNamingServiceimplementationGetArtifactNamingService()extension method onIServiceProviderTestHostBuilderHangDumpProcessLifetimeHandlerupdated to use the serviceVersionproperty added toIEnvironment/SystemEnvironmentdocs/ArtifactNamingService.mdNote
This PR revives and fixes the content from #6587 (closed copilot PR), with extensive code review fixes:
netstandard2.0(removedOperatingSystem.IsXxx(),KeyValuePairdeconstruction,[..8]range syntax)Guard.NotNullOrEmptyAPI%phandling<root>placeholder (directory tree traversal)EnvironmentAPI usage (RS0030)Fixes #6586
Related to #5364, #7345, #6648, #4130, #7126, #6778