Enlighten RequiresFramework35SP1Assembly task for multithreaded mode

[jankratochvilcz]

RequiresFramework35SP1Assembly is purely in-memory (string compares, Version parse/compare, ITaskItem metadata reads) — no filesystem, env vars, process start, or shared static state. Per the multithreaded-task-migration skill Step 1b, the [MSBuildMultiThreadableTask] attribute alone is sufficient; IMultiThreadableTask is not needed. No code changes to Execute or helpers, no targets change, no ChangeWave.

The task previously had no direct unit coverage. Added 14 test cases locking in the contract: defaults, each Execute predicate triggering independently, the CreateDesktopShortcut TFV ≥ v3.5 gate (both v-prefixed and bare version strings), and the SP1/Net35Client sentinel identity short-circuit. Passing on net10.0 and net472.

Fixes #13572

[Copilot]

Pull request overview

Decorates RequiresFramework35SP1Assembly as multithreadable (attribute-only) as part of the built-in task multithreaded migration, and adds direct unit tests to lock in existing ClickOnce-related predicate behavior.

Changes:

• Add [MSBuildMultiThreadableTask] to RequiresFramework35SP1Assembly.
• Add new unit tests covering the task’s independent trigger conditions (URL, shortcut + TFV gate, signing, suite name, and IncludeHash=false / sentinel identities).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/Tasks/RequiresFramework35SP1Assembly.cs	Marks the task as eligible for multithreaded execution via attribute.
src/Tasks.UnitTests/RequiresFramework35SP1Assembly_Tests.cs	Adds predicate-focused unit coverage for RequiresMinimumFramework35SP1 computation.

[github-actions]

Review Summary — Multithreaded Task Migration for RequiresFramework35SP1Assembly

Overall: Clean PR, correct migration. The task is purely in-memory (string compares, Version parse/compare, ITaskItem metadata reads) with no filesystem, environment variable, process, or shared static state access. The [MSBuildMultiThreadableTask] attribute alone (without IMultiThreadableTask) is the correct approach per the migration guidance.

✅ What looks good

Dimension	Assessment
Multithreaded migration	Correct — attribute-only pattern matches Message, Error, Warning, Telemetry, etc. No IMultiThreadableTask needed since there are no file/env operations.
Static state safety	Verified — ConvertFrameworkVersionToString and CompareFrameworkVersions are pure static functions. No shared mutable state.
Backwards compatibility	No behavior change — attribute is additive metadata only.
Test coverage	Strong — 14 tests covering defaults, each Execute predicate independently, version parsing edge cases (v-prefix and bare), SP1/sentinel identity short-circuit, and the CreateDesktopShortcut TFV gate.
Test isolation	Each test creates its own task instance via CreateTask(). No shared state between tests.
Concurrency	Task instance is not shared across threads. All fields are instance-scoped. Safe.
API surface	No public API changes — [MSBuildMultiThreadableTask] is detected by namespace/name, not assembly.
Naming / conventions	Test class follows {TaskName}_Tests convention. Method names describe behavior clearly.

📝 Minor suggestions (non-blocking)

1. #nullable disable on new file — Project guidelines say new files should use nullable reference types. Minor style point.
2. Missing negative test for benign items — A test with items present but not triggering (e.g., IncludeHash=true, non-matching identity) would strengthen the IsExcludedFileOrSP1File negative-path coverage.

No correctness issues, no breaking changes, no performance concerns.

Generated by Expert Code Review (on open) for issue #13575 · ● 1.9M