Avoid restoring SDK dependency anchors in CI - #10741
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 907e12cd-3058-464a-9f99-2b917a33a11a
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Review Summary
Clean PR — moves Dependabot anchor PackageReferences from the acceptance test project into a dedicated eng/Dependabot.Dependencies/Dependabot.Dependencies.csproj, eliminating unwanted transitive restore of heavy packages (Playwright) during test builds.
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | ✅ N/A — no logic changes |
| 2 | Edge-Case Robustness | ✅ N/A |
| 3 | Concurrency & Thread Safety | ✅ N/A |
| 4 | Performance & Allocation | ✅ N/A |
| 5 | Security | ✅ N/A |
| 6 | Public API Surface | ✅ No public API change |
| 7 | Backward Compatibility | ✅ No behavioral change |
| 8 | Error Handling & Observability | ✅ N/A |
| 9 | Cross-TFM Correctness | ✅ Single net8.0 TFM; only used as a restore anchor |
| 10 | Localization | ✅ N/A |
| 11 | Test Coverage | ✅ N/A — infrastructure-only |
| 12 | Test Quality | ✅ N/A |
| 13 | Code Style & Conventions | ✅ Follows repo conventions |
| 14 | Naming | ✅ Clear project name |
| 15 | Documentation & Comments | ✅ Thorough XML comments explaining the SolutionPath gating and why PackageDownload doesn't work |
| 16 | MSBuild Correctness | ✅ Build="false" in slnx + SolutionPath condition correctly isolates anchors from solution builds |
| 17 | IPC & Protocol | ✅ N/A |
| 18 | Dependency Management | ✅ Anchors use ExcludeAssets="all" and are CPM-managed |
| 19 | Configuration | ✅ N/A |
| 20 | Scope Discipline | ✅ Single concern — relocate Dependabot anchors |
| 21 | Licensing & Copyright | ✅ N/A |
| 22 | TODO Policy | ✅ No TODOs |
Overall: No issues found. The SolutionPath condition is a nice touch — it ensures the anchors are only restored when Dependabot runs project discovery (where SolutionPath is unset), preventing the large Playwright transitive closure from being downloaded during normal solution restores. The Build="false" marker in TestFx.slnx and the absence from .slnf filters are both correct.
🧵 Parallel-safety audit — PR #10741Nothing audited here touches process-global state, shared filesystem paths, or This PR changes only Audited Re-run with
|
🧪 Expert test review — PR #10741No new or modified test methods were identified in the changed regions of this PR. Nothing to review. Re-run with
|
There was a problem hiding this comment.
Pull request overview
Moves Dependabot-only dependency anchors out of the acceptance-test restore graph to reduce CI restore size.
Changes:
- Adds a non-building Dependabot dependency project.
- Excludes anchor packages from solution restores.
- Updates related dependency documentation.
Show a summary per file
| File | Description |
|---|---|
TestFx.slnx |
Registers the anchor project without building it. |
eng/Dependabot.Dependencies/Dependabot.Dependencies.csproj |
Hosts restore-isolated package anchors. |
MSTest.Acceptance.IntegrationTests.csproj |
Removes anchors from acceptance tests. |
Directory.Packages.props |
Updates anchor-location documentation. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
Summary
Microsoft.Playwright.MSTest.v4andAspire.Hosting.Testingreferences into a dedicated non-building projectTestFx.slnx, while preserving them for Dependabot's direct project discoveryThis avoids downloading Playwright's roughly 772 MB payload on every solution restore without disabling its dependency updates.
Validation
TestFx.slnxhas zero anchor dependencies, while the Dependabot project has bothMicrosoft.Playwright*packages