Address tech-debt batch: analyzer/report-engine dedup, MSBuild authoring, file diet#9487
Open
Evangelink wants to merge 2 commits into
Open
Address tech-debt batch: analyzer/report-engine dedup, MSBuild authoring, file diet#9487Evangelink wants to merge 2 commits into
Evangelink wants to merge 2 commits into
Conversation
, #9482, #9467, #9414) - #9483: extract RegisterInstanceFixtureAnalyzer helper shared by TestInitialize/TestCleanup analyzers - #9482: bundle report-engine dependencies into ReportEngineContext + CreateEngineContext factory - #9467: reclassify MSTEST0067 to Category.Performance, document Category taxonomy, add governance tests - #9414: declare Returns on GetMSTestV2CultureHierarchy; move _ValidateBundledSdkFeatureVersions target from Directory.Packages.props to Directory.Build.targets Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extract VideoSegment into its own file and split the recorder across partial files for process management and Windows window-capture, keeping each file well under 300 lines. No behavioral or API changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Jun 28, 2026
Closed
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a batch of tech-debt/quality items across MSTest analyzers, report-generation extensions, MSBuild authoring, and a large VideoRecorder source file, with the intent of reducing duplication and improving maintainability without changing behavior.
Changes:
- Deduplicated analyzer initialization code for instance fixture analyzers (
[TestInitialize]/[TestCleanup]) via a shared helper. - Introduced
ReportEngineContext+ a generator factory to remove repeated 10-argument report-engine construction across HTML/JUnit/CTRF (while keeping existing constructors). - Improved analyzer category taxonomy: documented category criteria, reclassified MSTEST0067 as
Performance, added governance tests; plus MSBuild authoring cleanups and VideoRecorder file split.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/MSTest.Analyzers.UnitTests/AnalyzerCategoryGovernanceTests.cs | Adds lightweight tests pinning analyzer Category values for performance-related rules |
| src/Analyzers/MSTest.Analyzers/AvoidThreadSleepAndTaskWaitInTestsAnalyzer.cs | Reclassifies MSTEST0067 from Usage to Performance |
| src/Analyzers/MSTest.Analyzers/Helpers/Category.cs | Documents decision criteria for Design/Performance/Usage taxonomy |
| src/Analyzers/MSTest.Analyzers/Helpers/FixtureMethodAnalyzerHelper.cs | Adds RegisterInstanceFixtureAnalyzer to centralize instance-fixture analyzer registration |
| src/Analyzers/MSTest.Analyzers/TestInitializeShouldBeValidAnalyzer.cs | Routes [TestInitialize] analyzer registration through shared helper |
| src/Analyzers/MSTest.Analyzers/TestCleanupShouldBeValidAnalyzer.cs | Routes [TestCleanup] analyzer registration through shared helper |
| src/Platform/SharedExtensionHelpers/ReportEngineContext.cs | Adds a context record bundling shared services + per-session arguments for report engines |
| src/Platform/SharedExtensionHelpers/ReportGeneratorBase.cs | Adds CreateEngineContext(...) factory to avoid repeated dependency lists in generators |
| src/Platform/SharedExtensionHelpers/ReportEngineBase.cs | Adds a context-based base constructor to thread dependencies through one parameter |
| src/Platform/Microsoft.Testing.Extensions.HtmlReport/Microsoft.Testing.Extensions.HtmlReport.csproj | Links ReportEngineContext.cs into HtmlReport extension build |
| src/Platform/Microsoft.Testing.Extensions.HtmlReport/HtmlReportGenerator.cs | Uses CreateEngineContext(...) to construct HtmlReportEngine |
| src/Platform/Microsoft.Testing.Extensions.HtmlReport/HtmlReportEngine.cs | Adds a context-taking constructor delegating to ReportEngineBase |
| src/Platform/Microsoft.Testing.Extensions.JUnitReport/Microsoft.Testing.Extensions.JUnitReport.csproj | Links ReportEngineContext.cs into JUnitReport extension build |
| src/Platform/Microsoft.Testing.Extensions.JUnitReport/JUnitReportGenerator.cs | Uses CreateEngineContext(...) to construct JUnitReportEngine |
| src/Platform/Microsoft.Testing.Extensions.JUnitReport/JUnitReportEngine.cs | Adds a context-taking constructor delegating to ReportEngineBase |
| src/Platform/Microsoft.Testing.Extensions.CtrfReport/Microsoft.Testing.Extensions.CtrfReport.csproj | Links ReportEngineContext.cs into CtrfReport extension build |
| src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportGenerator.cs | Uses CreateEngineContext(...) to construct CtrfReportEngine |
| src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.cs | Adds a context-taking constructor delegating to ReportEngineBase |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Microsoft.Testing.Extensions.TrxReport.csproj | Links ReportEngineContext.cs so ReportEngineBase continues to compile after new ctor overload |
| src/Platform/Microsoft.Testing.Extensions.VideoRecorder/FfmpegVideoRecorder.cs | Converts recorder to partial and removes extracted concerns from the main file |
| src/Platform/Microsoft.Testing.Extensions.VideoRecorder/FfmpegVideoRecorder.ProcessManagement.cs | Extracts ffmpeg process/args/exe-discovery code into its own partial file |
| src/Platform/Microsoft.Testing.Extensions.VideoRecorder/FfmpegVideoRecorder.WindowCapture.cs | Extracts Windows window-region capture + P/Invoke into its own partial file |
| src/Platform/Microsoft.Testing.Extensions.VideoRecorder/VideoSegment.cs | Extracts VideoSegment struct into its own file |
| src/Adapter/MSTest.TestAdapter/buildTransitive/common/MSTest.TestAdapter.targets | Adds Returns="@(MSTestV2Files)" to make target output contract explicit |
| src/Adapter/MSTest.TestAdapter/buildTransitive/uwp/MSTest.TestAdapter.targets | Adds Returns="@(MSTestV2Files)" to make target output contract explicit |
| Directory.Packages.props | Removes _ValidateBundledSdkFeatureVersions target from .props, updates comment to point to new location |
| Directory.Build.targets | Adds _ValidateBundledSdkFeatureVersions target in the correct .targets file |
Review details
- Files reviewed: 27/27 changed files
- Comments generated: 1
- Review effort level: Low
Comment on lines
+4
to
+25
| using System.Linq; | ||
|
|
||
| namespace MSTest.Analyzers.Test; | ||
|
|
||
| /// <summary> | ||
| /// Lightweight governance tests that pin the diagnostic <c>Category</c> taxonomy for performance-impacting | ||
| /// analyzers so misclassifications are caught at build time without any Roslyn workspace overhead. | ||
| /// See https://github.com/microsoft/testfx/issues/9467. | ||
| /// </summary> | ||
| [TestClass] | ||
| public sealed class AnalyzerCategoryGovernanceTests | ||
| { | ||
| private const string PerformanceCategory = "Performance"; | ||
|
|
||
| [TestMethod] | ||
| public void UseParallelizeAttribute_MainRule_UsesPerformanceCategory() | ||
| => Assert.AreEqual(PerformanceCategory, UseParallelizeAttributeAnalyzer.Rule.Category); | ||
|
|
||
| [TestMethod] | ||
| public void AvoidThreadSleepAndTaskWaitInTests_UsesPerformanceCategory() | ||
| => Assert.AreEqual(PerformanceCategory, new AvoidThreadSleepAndTaskWaitInTestsAnalyzer().SupportedDiagnostics.Single().Category); | ||
| } |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Comment on lines
+68
to
+69
| { | ||
| } |
Comment on lines
+92
to
+93
| { | ||
| } |
| public static readonly IntPtr DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2 = new(-4); | ||
|
|
||
| [DllImport("kernel32.dll")] | ||
| public static extern IntPtr GetConsoleWindow(); |
| public static extern IntPtr GetConsoleWindow(); | ||
|
|
||
| [DllImport("user32.dll")] | ||
| public static extern IntPtr GetForegroundWindow(); |
| public static extern IntPtr GetForegroundWindow(); | ||
|
|
||
| [DllImport("user32.dll")] | ||
| public static extern int GetSystemMetrics(int nIndex); |
| public static extern int GetSystemMetrics(int nIndex); | ||
|
|
||
| [DllImport("user32.dll")] | ||
| public static extern IntPtr SetThreadDpiAwarenessContext(IntPtr dpiContext); |
|
|
||
| [DllImport("user32.dll")] | ||
| [return: MarshalAs(UnmanagedType.Bool)] | ||
| public static extern bool IsWindowVisible(IntPtr hWnd); |
|
|
||
| [DllImport("user32.dll", SetLastError = true)] | ||
| [return: MarshalAs(UnmanagedType.Bool)] | ||
| public static extern bool GetWindowRect(IntPtr hWnd, out RECT lpRect); |
| IntPtr previousDpiContext = TrySetPerMonitorDpiAwareThread(); | ||
| try | ||
| { | ||
| int screenWidth = NativeMethods.GetSystemMetrics(NativeMethods.SM_CXSCREEN); |
| try | ||
| { | ||
| int screenWidth = NativeMethods.GetSystemMetrics(NativeMethods.SM_CXSCREEN); | ||
| int screenHeight = NativeMethods.GetSystemMetrics(NativeMethods.SM_CYSCREEN); |
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 a batch of automated tech-debt / quality issues. Each change is validated with a clean full build (
build.cmd -c Debug: 0 warnings, 0 errors) and the affected unit-test suites pass.Implemented
#9483 — Duplicate
TestInitialize/TestCleanupanalyzersExtracted
FixtureMethodAnalyzerHelper.RegisterInstanceFixtureAnalyzerand routed both analyzers through it. TheConfigureGeneratedCodeAnalysis/EnableConcurrentExecutioncalls stay in eachInitialize(required by RS1025/RS1026). PublicRulefields are unchanged.#9482 — Duplicate report-engine constructor arguments
Added
ReportEngineContext(inSharedExtensionHelpers) plus aReportGeneratorBase.CreateEngineContext(...)factory and a context-acceptingReportEngineBaseconstructor. The three generators (Html/JUnit/Ctrf) now build their engine from a single context instead of repeating the 10-argument list. The existing 10-parameter engine constructors are kept so the engine unit tests are untouched.#9467 — Analyzer
Category.Performancedeficit (partial — safe subset)AvoidThreadSleepAndTaskWaitInTests) toCategory.Performance— non-breaking because the rule is disabled by default.Categorytaxonomy decision criteria inCategory.cs.AnalyzerCategoryGovernanceTestspinning MSTEST0001 and MSTEST0067 toPerformance.Category.Design→Performance). Both affect default-enabled behavior for existing consumers and the issue itself flags MSTEST0045 as needing a changelog/next-release. Happy to do them in a follow-up if maintainers want.#9414 — MSBuild file quality (remaining warnings)
Returns="@(MSTestV2Files)"onGetMSTestV2CultureHierarchyin both thecommonanduwpMSTest.TestAdapter.targets._ValidateBundledSdkFeatureVersionsfromDirectory.Packages.propsintoDirectory.Build.targets(targets belong in.targets).#9464 — File diet for
FfmpegVideoRecorder.cs(723 → 354 lines)Extracted
VideoSegmentinto its own file and split the recorder into partial files:FfmpegVideoRecorder.ProcessManagement.csandFfmpegVideoRecorder.WindowCapture.cs. No behavioral or API changes.Already fixed / in progress (linked, not re-done here)
IsTypeReachableFromGeneratedCodealready removed).perf-timing-nightly.yml).Deferred
BannerMessageHelperandOutputDeviceBannerHelper. Unifying them is non-trivial: the two banners intentionally differ (culture-sensitive vs invariant date, commit-hash presence, injected vs computed arch) and live in different assemblies with a layering constraint, so a merge risks changing user-visible banner output and the--help/--infoacceptance snapshots. Left for a deliberate follow-up.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com