[docs] Address glossary review feedback from PR #8951#8953
Merged
Conversation
Fixes the four issues raised in code review of #8951: - ArtifactNamingHelper: removed the misleading 'legacy %p' sentence from the helper's description (the helper only understands {placeholder} tokens; %p substitution is performed by HangDumpProcessLifetimeHandler as a post-processing step after ArtifactNamingHelper.ResolveTemplate). Also corrected the list of consumers: the helper is used by HangDump and by the report extensions (HtmlReport, JUnitReport, TrxReport) via ReportFileNameHelper, not by CrashDump. - JUnitReport: removed the incorrect claim that the extension auto-registers via an EnableMicrosoftTestingExtensionsJUnitReport MSBuild property. That property exists only as a proposal in docs/RFCs/016-JUnit-Report.md; the actual implementation unconditionally registers a TestingPlatformBuilderHook from its buildTransitive props, so adding the package reference is sufficient. - testconfig.json: fixed the broken file reference. The correct path is docs/microsoft.testing.platform/002-TestConfig-EnvironmentVariables.md; docs/RFCs/002-... is 002-Framework-Extensibility-Custom-Assertions.md, a completely unrelated RFC. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This docs-only PR updates docs/glossary.md to correct a few recently added glossary entries, aligning them with the actual implementation in Microsoft.Testing.Platform and its extensions.
Changes:
- Clarifies
ArtifactNamingHelperbehavior and consumers (removes misleading%pimplication; corrects CrashDump vs HangDump/report usage). - Fixes
JUnitReportregistration description to match how the package registers via MSBuild hooks (no opt-in property). - Corrects the referenced RFC/document path for
testconfig.jsonenvironmentVariables.
Show a summary per file
| File | Description |
|---|---|
| docs/glossary.md | Corrects glossary entry inaccuracies for ArtifactNamingHelper, JUnitReport, and testconfig.json references. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 2
This was referenced Jun 9, 2026
Two Copilot bot review comments on #8953: - ArtifactNamingHelper entry: linked TrxReport (other report extensions were already linked). - JUnitReport entry: corrected 'declared in buildTransitive props' -> declared in buildMultiTargeting (the build/ and buildTransitive/ props only import that file). Cross-PR reconciliation: - ArtifactNamingHelper entry: re-added CrashDump as a direct consumer and documented the {pid}->%p / {pname}->%e deferred-mapping pattern, matching #8957 which actually wires CrashDump into ArtifactNamingHelper. Avoids a conflict between #8953 and #8957 on this same line. - JUnitReport entry: kept the 'no opt-in property at the package level' note for direct package consumers, and added a paragraph documenting the MSTest.Sdk-level <EnableMicrosoftTestingExtensionsJUnitReport> opt-in introduced in #8956. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
Pushed c81333a to address the two Copilot bot review comments and to harmonize this PR with two newly-opened follow-ups:
The three PRs (#8953, #8956, #8957) are now consistent and should not conflict on overlapping lines. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the four review comments from #8951 (now merged), which highlighted inaccuracies in the new glossary entries.
Changes
ArtifactNamingHelperentry%ppatterns from earlier hang-dump versions continue to work." from the helper''s description. The helper only understands{placeholder}tokens — the%psubstitution is performed byHangDumpProcessLifetimeHandleras a separate post-processing step afterArtifactNamingHelper.ResolveTemplateis called.ReportFileNameHelper. CrashDump does not use it — it relies on the runtimecreatedumpplaceholders (%p,%t, etc.).%psubstitution lives in the HangDump extension, not in the helper.JUnitReportentry<EnableMicrosoftTestingExtensionsJUnitReport>MSBuild property. That property does not exist in the implementation — it only appears as a proposal indocs/RFCs/016-JUnit-Report.md. The shipped extension unconditionally registers aTestingPlatformBuilderHookfrom itsbuildTransitiveprops, so adding the package reference is sufficient.testconfig.jsonentrydocs/RFCs/002-TestConfig-EnvironmentVariables.mddoes not exist;docs/RFCs/002-...is002-Framework-Extensibility-Custom-Assertions.md, a completely unrelated RFC. The actual document lives atdocs/microsoft.testing.platform/002-TestConfig-EnvironmentVariables.md.Review threads addressed
%pattribution)docs/RFCs/002-...path)Docs-only change.