Unit testing skill#13574
Conversation
🔍 Skill Validator Results✅ All checks passed
Summary
Full validator output```text Found 1 skill(s) [running-unit-tests] 📊 running-unit-tests: 2,376 BPE tokens [chars/4: 2,310] (detailed ✓), 10 sections, 6 code blocks ✅ All checks passed (1 skill(s)) ``` |
There was a problem hiding this comment.
Pull request overview
Adds a new “running-unit-tests” skill document under .github/skills/ to reduce friction and cost when running MSBuild unit tests, especially around MTP + xUnit v3 specifics and efficient iteration/validation workflows.
Changes:
- Introduces repo-specific guidance for running unit tests with xUnit v3 on Microsoft.Testing.Platform (MTP).
- Documents recommended fast dev-loop commands (single TFM,
--no-build/--no-restore, direct exe runs) vs. heavier validation passes. - Captures common flag pitfalls (VSTest-only options) and highlights repo-wide defaults (trait filters, parallelism settings, coverage behavior).
There was a problem hiding this comment.
Review Summary — Unit Testing Skill
Overall this is a well-written, high-value skill document that addresses a real friction point (agents wasting time on incorrect test runner flags). The content is thorough and the dev-loop vs. final-validation structure is excellent. A few accuracy issues should be fixed before merge.
Dimension Summary
| # | Dimension | Verdict | Notes |
|---|---|---|---|
| 1 | Backwards Compatibility | ✅ LGTM | N/A — doc-only PR |
| 2 | ChangeWave Discipline | ✅ LGTM | N/A |
| 3 | Performance & Allocation | ✅ LGTM | N/A |
| 4 | Test Coverage & Completeness | ✅ LGTM | N/A — describes test running, not test code |
| 5 | Error Message Quality | ✅ LGTM | N/A |
| 6 | Logging & Diagnostics | ✅ LGTM | N/A |
| 7 | String Comparison | ✅ LGTM | N/A |
| 8 | API Surface Discipline | ✅ LGTM | N/A |
| 9 | Target Authoring | ✅ LGTM | N/A |
| 10 | Design Before Implementation | ✅ LGTM | Linked to #13567; scope is appropriate |
| 11 | Cross-Platform Correctness | All examples use \ paths and PowerShell; forward slashes are more portable |
|
| 12 | Code Simplification | ✅ LGTM | N/A |
| 13 | Concurrency & Thread Safety | ✅ LGTM | N/A |
| 14 | Naming Precision | ✅ LGTM | Consistent with repo conventions |
| 15 | SDK Integration | ✅ LGTM | N/A |
| 16 | Idiomatic C# Patterns | ✅ LGTM | N/A |
| 17 | File I/O & Path Handling | ✅ LGTM | N/A |
| 18 | Documentation Accuracy | Broken table (lines 33–41); wrong src/Directory.Build.props path; nonexistent MSBuildTestAssemblyFixture; non-functional -p:CollectCoverage=false |
|
| 19 | Build Infrastructure Care | ✅ LGTM | N/A |
| 20 | Scope & PR Discipline | Missing argument-hint frontmatter field vs. other skills |
|
| 21 | Evaluation Model Integrity | ✅ LGTM | N/A |
| 22 | Correctness & Edge Cases | Factual claims don't match codebase (see inline comments) | |
| 23 | Dependency Management | ✅ LGTM | N/A |
| 24 | Security Awareness | ✅ LGTM | N/A |
Key Findings (in priority order)
-
Broken Markdown table (MODERATE, line 33–41): A paragraph interrupts the flags table, causing 6 rows to render as raw text instead of table cells. Easy fix — move the paragraph after the table.
-
Wrong
Directory.Build.propspath (MODERATE, line 14):TestRunnerName,XUnitV3Version,MicrosoftTestingPlatformVersion, andOutputTypeare in the rootDirectory.Build.props, notsrc/Directory.Build.props. -
MSBuildTestAssemblyFixturenot found (MODERATE, line 21): This class doesn't appear to exist in the codebase.DOTNET_HOST_PATHis set byRunnerUtilities.GetMSBuildEnvironmentVariables()insrc/UnitTests.Shared/RunnerUtilities.cs. -
-p:CollectCoverage=falseis a no-op (NIT, line 99): This property isn't recognized by the build infrastructure. Coverage is controlled viaXunitOptionsinsrc/Directory.Build.targets. -
Windows-only path examples (NIT, lines 62–73): Forward slashes work everywhere with
dotnetCLI and would be more portable.
No blocking issues. The broken table is the most impactful since it hides important information from readers.
Generated by Expert Code Review (on open) for issue #13574 · ● 6.5M
- Add argument-hint frontmatter for consistency with other skills - Correct file references (TestRunnerName etc. live in repo-root Directory.Build.props, not src/) - Replace MSBuildTestAssemblyFixture (does not exist) with the real mechanism: RunnerUtilities.GetMSBuildEnvironmentVariables - Reword 'forwards unknown args' since dotnet test does not generally forward arbitrary flags to MTP - Fix broken Markdown table: move the MTP-native paragraph below the full table - Drop -p:CollectCoverage=false suggestion (not wired up); document that direct exe is the only supported way to skip --coverage - Add cross-platform note for paths and exe extension on Linux/macOS Co-authored-by: Copilot <[email protected]>
|
FYI @Evangelink |
|
I believe that most of the issues described here would be handled by the test skills in dotnet/skills repo. It'd be great if this could be dogfooded instead of using something custom. We can help with setting up a sync bot if you are interested. |
|
Ideally GH would natively support the marketplace/plugin ecosystem... Not sure if anything is planned on that front. |
Agreed. We should avoid duplicity, building something custom and dogfood the shipping skills, ideally. TIL that you can add this file to the repo to enable a plugin by default:
Not sure if CCA (copilot coding agent) respects that though. dotnet/dotnet@6e5b009 |
|
Thanks for the feedback @ViktorHofer @Evangelink! If you don't mind I'd still like to merge this. There is some custom things here (the cmd script etc.,), I found this skill to make a difference. I will try to experiment with only the built-in skills in next PRs and if it still solves the problem happy to use those, make them default, and remove this one :-) |
|
@jankratochvilcz just a quick note that you are basically imposing this skill to everyone which is equivalent to enabling a plugin (agent + skills) by default. |
|
@Evangelink it's a fair point. This said, it's a specific skill that solves a specific pain point within this repo; that's why I feel it's justified to add it compared to activating the bundles by default. Would you have another take? An alternative would be to not git-check it and keep it on my machine boundary. |
rainersigwald
left a comment
There was a problem hiding this comment.
I'm willing to experiment with something like this.
|
That works. What @Evangelink and I are eluding to is that we put significant time and effort into writing focused, shipping skills in the dotnet/skills repository that target customers. We depend on dogfooding and 1st party engagement here and would love for repositories like msbuild to have them enabled by default so that we can improve them. By that, we can have positive impact on a larger customer base than the msbuild team. |
Add `.github/copilot/settings.json` to configure Copilot agent plugin marketplaces and enable dotnet plugins: - `dotnet/arcade-skills` marketplace with `dotnet-dnceng` plugin - `dotnet/skills` marketplace with `dotnet`, `dotnet-msbuild`, `dotnet-nuget`, and `dotnet-test` plugins Relates to (and should replace a big part of) #13574

I noticed a lot of friction when running unit tests via the agent for #13567. This skill is mostly opinion-agnostic and tries to makes sure that the agent doesn't get stuck running tests with wrong configuration, which by default is super expensive wall click-wise. It also adds some hints to prevent always running multi-target during dev and only doing that for validation, which also cuts down the time needed for iteration.