Backport genuinely-missing main commits to rel/4.3#9692
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> (cherry picked from commit fe518a5)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> (cherry picked from commit 8c167ed)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> (cherry picked from commit 621b8ff)
🧪 Test quality grade — PR #9692
This advisory comment was generated automatically. Grades are heuristic
|
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Expert Review — PR #9692
| Dimension | Severity | Verdict | Notes |
|---|---|---|---|
| 1. Algorithmic Correctness | MAJOR | ✅ | GetInvokeResultWithParametersAsync correctly threads the cached ParameterInfo[] through all call sites. ConstructGenericMethod signature and internal usage updated consistently. |
| 2. Threading & Concurrency | BLOCKING | ✅ | TestMethodInfo.ParameterTypes uses benign lazy-init (field ??=); worst case is double-compute of an equivalent array. No shared-mutable-state concerns. |
| 3. Security & IPC Contract Safety | BLOCKING | ✅ | No new deserialization or IPC changes. Workflow uses contents: read at top level with write scoped to the update job only. |
| 4. Public API & Binary Compatibility | BLOCKING | ✅ | All new/changed methods are internal. No public surface changes. |
| 5. Performance & Allocations | MAJOR | ✅ | Core goal of the PR — GetParameters() allocation eliminated from the hot path by caching via ParameterTypes property. Clean split between hot-path (GetInvokeResultWithParametersAsync) and non-hot (GetInvokeResultAsync wrapper). |
| 6. Cross-TFM Compatibility | MAJOR | ✅ | Changes compile across all TFMs (net462/netstandard2.0/net8.0/net9.0). No TFM-specific APIs introduced. |
| 7. Test Coverage & Quality | MAJOR | ✅ | New analyzer edge-case tests follow existing patterns (VerifyAnalyzerAsync/VerifyCodeFixAsync). Localization tests re-enabled. |
| 8. Error Handling & Diagnostics | MAJOR | ✅ | RuntimeError raised with context in the Python script. C# error paths unchanged (same TestFailedException messages). |
| 9. Naming & Style | MINOR | ✅ | arguments == null → arguments is null ✓. methodParameters?.Length ?? 0 → methodParameters.Length (now non-nullable) ✓. |
| 10. Documentation & Comments | MINOR | ✅ | Good inline comments explaining the thin-wrapper pattern in MethodInfoExtensions. README updated with 8-day PAT policy and fast-unblock instructions. |
| 11. Resource & Lifecycle Management | MAJOR | ✅ | N/A — no IDisposable or CancellationToken flow changes. |
| 12. Localization | MAJOR | ✅ | N/A — no new user-facing strings. |
| 13. Build & Project File Quality | MAJOR | ✅ | N/A — no .csproj / .props / .targets changes. |
| 14. Backward / Forward Compatibility | MAJOR | ✅ | N/A — no wire-format changes. |
| 15. Dependency Hygiene | MAJOR | ✅ | N/A — no new package references. |
| 16. Changelog / Release Notes | MINOR | ✅ | #9641 entry added. Old testanywhere links corrected to testfx. |
| 17. Agentic Workflow Quality | MAJOR | ✅ | N/A — no agentic workflow .md sources changed. |
| 18. TODO Comment Policy | MINOR | ✅ | No bare TODO comments introduced. |
| 19. CI/CD & Workflow Correctness | MAJOR | ✅ | Actions pinned to full SHAs (actions/checkout@34e11..., actions/setup-python@a26af..., peter-evans/create-pull-request@67ccf...). Least-privilege permissions. Concurrency control prevents racing updates. |
| 20. Inline Test Infrastructure | MAJOR | ✅ | N/A — no acceptance test duration assertions. |
| 21. MSBuild / SDK Quality | MAJOR | ✅ | N/A — no props/targets changes. |
| 22. Python / Shell Script Quality | MAJOR | Minor: _stable_tuple(current) can return None causing TypeError if the props file ever holds a prerelease string. Low practical risk. See inline comment. |
Overall Verdict: LGTM
Well-crafted backport. The perf optimization in MethodInfoExtensions is the highest-impact change — it correctly eliminates per-invocation GetParameters() allocations on the data-driven test hot path by threading a lazily-cached ParameterInfo[] from TestMethodInfo.ParameterTypes. The new Playwright bump workflow follows all CI security best practices. Test additions are thorough edge-case coverage for existing analyzers.
Backports the genuinely-missing (non-vstest-removal) commits from
maintorel/4.3. Every commit was cherry-picked with-x; the full solution builds clean (build.cmd -c Debug: 0 warnings, 0 errors).Backported (8)
Perf
Tests
Docs / infra
::warningannotations for skipped tests in GitHubActionsReport #9641 entry (docs: fix Changelog-Platform links and add #9641 entry #9667) — resolved conflict to add only the Emit::warningannotations for skipped tests in GitHubActionsReport #9641 entry (already backported); dropped the main-only Add --report-azdo-groups/--report-azdo-annotations on|off toggles #9542/Re-print errored assemblies in dotnet test end-of-run recap #9545/Add server-initiated session cancellation to the dotnet test IPC protocol (#8691) #9549 entriesIntentionally excluded
GetOrCreateHostTestCase(), introduced by the un-backported vstest decoupling (Relocate the UnitTestElement<->TestCase conversion to the adapter (Phase 6e-3b) #9626)thisinstead of the clone #9581) that never shipped in rel/4.3[main] Update dependenciesbumps and OneLocBuild check-ins