Skip to content

Fix MSBuild import consistency findings (#9761)#9830

Closed
Evangelink wants to merge 3 commits into
mainfrom
dev/amauryleve/stunning-meme
Closed

Fix MSBuild import consistency findings (#9761)#9830
Evangelink wants to merge 3 commits into
mainfrom
dev/amauryleve/stunning-meme

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Fixes #9761

Addresses the two 🔵 suggestions from the MSBuild File Quality Report.

Changes

Microsoft.Testing.Extensions.VideoRecorder/buildTransitive/…props (Rule E-1)

Switched from importing directly out of buildMultiTargeting/ to the standard build/<tfm>/ forwarding pattern used by all 13 other extensions:

<Import Project="$(MSBuildThisFileDirectory)..\..\build\$([System.IO.Path]::GetFileName($([System.IO.Path]::GetDirectoryName($(MSBuildThisFileDirectory)))))\Microsoft.Testing.Extensions.VideoRecorder.props" />

Verified equivalent: in the package, buildTransitive/<tfm>/ forwards to build/<tfm>/, whose build/…props in turn forwards to buildMultiTargeting/…props — the same real content the old direct import resolved to. Purely a consistency change.

Microsoft.Testing.Platform.MSBuild/buildMultiTargeting/…targets (Rule D-1)

Prefixed the CustomTestTarget.targets import with $(MSBuildThisFileDirectory) for consistency with the adjacent VSTest.targets import. Functionally identical (MSBuild resolves bare relative imports against the importing file's directory).

Both changes are non-functional / cosmetic consistency fixes with no behavioral impact.

- VideoRecorder buildTransitive: use the standard build/<tfm> forwarding pattern used by all other extensions (resolves to the same buildMultiTargeting content).
- Platform.MSBuild buildMultiTargeting: prefix CustomTestTarget import with $(MSBuildThisFileDirectory) for consistency with the adjacent import.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 10, 2026 15:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Standardizes two MSBuild imports without changing behavior.

Changes:

  • Aligns VideoRecorder’s transitive forwarding with other extensions.
  • Makes the custom test-target import path explicit.
Show a summary per file
File Description
Microsoft.Testing.Platform.MSBuild.targets Prefixes the custom target import with its file directory.
Microsoft.Testing.Extensions.VideoRecorder.props Uses the standard TFM-specific forwarding path.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Medium

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This review was generated by Copilot from workflow run #29104665282.

Summary: Non-functional consistency fixes to MSBuild import paths. Both changes are correct and low-risk.

Microsoft.Testing.Platform.MSBuild.targets (Rule D-1): Adding $(MSBuildThisFileDirectory) prefix is a safe consistency improvement. MSBuild resolves bare relative imports against the importing file's directory anyway, so this is purely cosmetic alignment with the adjacent VSTest.targets import on line 314.

VideoRecorder buildTransitive props (Rule E-1): The TFM-extraction pattern ($([System.IO.Path]::GetFileName($([System.IO.Path]::GetDirectoryName(...))))) is the same forwarding idiom used by the other 13 extensions. The double backslash (\\) after the extracted TFM folder name is intentional — MSBuild property functions need the escape. The resolved path (build/<tfm>/Microsoft.Testing.Extensions.VideoRecorder.props) ultimately chains back to the same buildMultiTargeting content, so behavior is unchanged.

No concerns — LGTM.

@github-actions

Copy link
Copy Markdown
Contributor

🔴 Build Failure Analysis

Root Cause

The build failure is not caused by this PR's changes. This PR only modifies MSBuild import paths in .props/.targets files. The errors originate from FileLoggerTests.cs, which was modified by the base branch commit c66515a (PR #9187 — "RFC 018: Artifact post-processing for dotnet test").

Errors (4 instances, 1 root cause)

Code File Lines Message
CA1416 FileLoggerTests.cs 465, 490 ITask.RunLongRunning(...) is unsupported on browser

The ITask.RunLongRunning interface method is annotated with [UnsupportedOSPlatform("browser")]. Two test helper classes (DelegatingTask at line 465 and NeverCompletingTask at line 490) implement this method and call through to _inner.RunLongRunning(...), triggering CA1416 because the test project doesn't restrict its supported platforms.

Suggested Fix (on the base branch)

Add #pragma warning disable CA1416 around the two RunLongRunning implementations in test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/FileLoggerTests.cs:

#pragma warning disable CA1416 // Platform compatibility - implementing interface method
public Task RunLongRunning(Func<Task> action, string name, CancellationToken cancellationToken)
    => _inner.RunLongRunning(action, name, cancellationToken);
#pragma warning restore CA1416

Or, alternatively, add a project-level suppression in the test .csproj for CA1416 on browser since the test project will never run in a browser context.

Impact on This PR

This PR's MSBuild import path fixes are correct and unrelated to the failure. The build will pass once the CA1416 issue is resolved on the base branch (or this PR carries the fix forward).

🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 59.5 AIC · ⌖ 5.67 AIC · ⊞ 7.3K · [◷]( · )

The DelegatingTask and NeverCompletingTask test helpers implement
ITask.RunLongRunning, which is annotated [UnsupportedOSPlatform('browser')].
Since the test project declares browser as a supported platform but never
runs in a browser, wrap the two overrides in #pragma warning disable/restore
CA1416, matching the existing suppression style already used in this file.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1b42ef96-9977-4d73-b1a0-6bd952777a4c
Copilot AI review requested due to automatic review settings July 11, 2026 12:29
@Evangelink

Copy link
Copy Markdown
Member Author

Addressed in a0c988c. The CA1416 build failure originated on the base branch (the DelegatingTask/NeverCompletingTask helpers in FileLoggerTests.cs implement ITask.RunLongRunning, which is annotated [UnsupportedOSPlatform("browser")]). I've wrapped both overrides in #pragma warning disable/restore CA1416 — matching the suppression style already used elsewhere in that file — so CI on this PR is now green (verified locally: Microsoft.Testing.Platform.UnitTests builds with 0 warnings / 0 errors across net8.0, net9.0, and net462).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

…ng-meme

# Conflicts:
#	test/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/FileLoggerTests.cs
Copilot AI review requested due to automatic review settings July 11, 2026 12:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

@Evangelink Evangelink closed this Jul 11, 2026
@Evangelink
Evangelink deleted the dev/amauryleve/stunning-meme branch July 11, 2026 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[msbuild-quality] MSBuild File Quality Report — 2026-07-09

2 participants