Uh oh!
There was an error while loading. Please reload this page.
Backport genuinely-missing main commits to rel/4.3 - #9692
Backport genuinely-missing main commits to rel/4.3#9692Amaury Levé (Evangelink) merged 8 commits into
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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