Improve Copilot guidance for R2R tests and regression verification#125833
Improve Copilot guidance for R2R tests and regression verification#125833PureWeen wants to merge 2 commits intodotnet:mainfrom
Conversation
- Promote regression test verification to a mandatory section in copilot-instructions.md with explicit guidance on using baseline build artifacts for negative verification - Add ReadyToRun/crossgen2 test gotchas section covering --inputbubble, map entry semantics, and MarshalHelpers.cs string matching - Add cross-component build warning (crossgen2 + CoreLib changes) - Add design doc discoverability instruction - Strengthen CoreLib rebuild guidance to explain libs.pretest necessity - Add Step 5 (mandatory negative verification) to jit-regression-test skill - Create src/tests/readytorun/README.md documenting test patterns, map validation rules, version bubble constraints, and platform gating Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add P/Invoke marshalling and R2R-specific review rules: - inputbubble requirement for cross-module CoreLib references - IsMarshallingRequired prevents JIT from inlining past R2R stubs - Separation of marshalling vs platform-specific stub requirements - DllImport detection string matching against MarshalHelpers.cs - R2R map validation (MethodWithGCInfo vs MethodFixupSignature) - R2R+NativeAOT P/Invoke emitter alignment checks - R2R tests need both map validation and runtime verification Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
There was a problem hiding this comment.
Pull request overview
This PR updates Copilot guidance (instructions + skills) and adds documentation to improve authoring/reviewing ReadyToRun (R2R)/crossgen2 tests, with an emphasis on mandatory regression-test verification (fail without fix, pass with fix).
Changes:
- Strengthens
.github/copilot-instructions.mdwith mandatory regression-test verification, R2R/crossgen2 gotchas, and additional troubleshooting guidance. - Updates Copilot skills (
jit-regression-test,code-review) to include mandatory negative verification and R2R-specific review rules. - Adds a new
src/tests/readytorun/README.mddocumenting R2R test patterns, map validation semantics, and build/run instructions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
.github/copilot-instructions.md |
Adds/strengthens guidance for regression verification, R2R/crossgen2 test authoring, and troubleshooting. |
.github/skills/jit-regression-test/SKILL.md |
Adds a mandatory step requiring negative (pre-fix) and positive (post-fix) verification of regression tests. |
.github/skills/code-review/SKILL.md |
Adds R2R-focused review rules, including map validation semantics and P/Invoke emitter alignment checks. |
src/tests/readytorun/README.md |
New R2R test authoring doc: harness vs manual crossgen2 patterns, map entry semantics, and build/run instructions. |
| - Examples: | ||
| - `tests/mainv1.csproj` | ||
| - `determinism/crossgen2determinism.csproj` | ||
| - `ObjCPInvokeR2R/ObjCPInvokeR2R.csproj` | ||
|
|
There was a problem hiding this comment.
ObjCPInvokeR2R/ObjCPInvokeR2R.csproj is referenced as an example, but there is no ObjCPInvokeR2R directory/test under src/tests/readytorun/ in this branch. This will confuse readers; either update the example to point to an existing test in this folder or add a note that this example comes from an out-of-tree/other PR.
| - **`MethodFixupSignature`** entries are metadata or signature references. They do not prove that the method body was compiled. | ||
| - **`DelayLoadHelperImport`** entries are call-site fixups. They are also metadata-related and are not proof of compilation. | ||
|
|
||
| Always check for `MethodWithGCInfo` when asserting that a method was precompiled into the R2R image. `ObjCPInvokeR2R/ObjCPInvokeR2R.cs` and `tests/test.cs` both use this rule when validating map output. |
There was a problem hiding this comment.
This sentence cites ObjCPInvokeR2R/ObjCPInvokeR2R.cs as an existing example of MethodWithGCInfo map validation, but that file doesn't exist under src/tests/readytorun/ in this branch. Consider referencing only tests/test.cs (which does contain a MethodWithGCInfo check) or another real file in this directory.
| Always check for `MethodWithGCInfo` when asserting that a method was precompiled into the R2R image. `ObjCPInvokeR2R/ObjCPInvokeR2R.cs` and `tests/test.cs` both use this rule when validating map output. | |
| Always check for `MethodWithGCInfo` when asserting that a method was precompiled into the R2R image. For example, `tests/test.cs` uses this rule when validating map output. |
| In practice, this is why tests like `ObjCPInvokeR2R` use manual crossgen2 invocation with `--inputbubble`. | ||
|
|
||
| ## Platform Gating | ||
|
|
||
| Use MSBuild conditions to keep ReadyToRun tests targeted to the environments they actually validate. | ||
|
|
||
| - Use `<CLRTestTargetUnsupported Condition="...">true</CLRTestTargetUnsupported>` for platform-specific tests. | ||
| - Example: `ObjCPInvokeR2R.csproj` uses `Condition="'$(TargetsOSX)' != 'true'"` because `objc_msgSend` behavior is only relevant on Apple platforms. |
There was a problem hiding this comment.
The platform-gating example refers to ObjCPInvokeR2R.csproj and a specific $(TargetsOSX) condition, but there is no such project under src/tests/readytorun/ in this branch. Please update the example to reference an existing ReadyToRun test project in this directory that uses CLRTestTargetUnsupported/DisableProjectBuild, or remove the file-specific example.
| In practice, this is why tests like `ObjCPInvokeR2R` use manual crossgen2 invocation with `--inputbubble`. | |
| ## Platform Gating | |
| Use MSBuild conditions to keep ReadyToRun tests targeted to the environments they actually validate. | |
| - Use `<CLRTestTargetUnsupported Condition="...">true</CLRTestTargetUnsupported>` for platform-specific tests. | |
| - Example: `ObjCPInvokeR2R.csproj` uses `Condition="'$(TargetsOSX)' != 'true'"` because `objc_msgSend` behavior is only relevant on Apple platforms. | |
| In practice, this is why some tests in this directory use manual crossgen2 invocation with `--inputbubble` instead of the built-in harness. | |
| ## Platform Gating | |
| Use MSBuild conditions to keep ReadyToRun tests targeted to the environments they actually validate. | |
| - Use `<CLRTestTargetUnsupported Condition="...">true</CLRTestTargetUnsupported>` for platform-specific tests. | |
| - Example condition: `Condition="'$(TargetsOSX)' != 'true'"` for tests that rely on macOS-specific behavior such as `objc_msgSend`. |
| src/tests/build.sh checked -tree:src/tests/readytorun | ||
|
|
||
| # Generate Core_Root layout (required for manual runs) | ||
| src/tests/build.sh -GenerateLayoutOnly x64 Release | ||
|
|
||
| # Run a single test manually | ||
| export CORE_ROOT=$(pwd)/artifacts/tests/coreclr/<os>.<arch>.Release/Tests/Core_Root | ||
| cd artifacts/tests/coreclr/<os>.<arch>.Debug/readytorun/<TestName>/ |
There was a problem hiding this comment.
The build/run snippet mixes configurations: it builds tests as checked, generates Core_Root as Release, and then cds into a Debug test output directory while running with the Release Core_Root. This is likely to produce incorrect instructions and hard-to-diagnose failures. Please make the example consistent (e.g., use a single <buildType> placeholder for both build output and CORE_ROOT, and cd into the matching <buildType> directory).
| src/tests/build.sh checked -tree:src/tests/readytorun | |
| # Generate Core_Root layout (required for manual runs) | |
| src/tests/build.sh -GenerateLayoutOnly x64 Release | |
| # Run a single test manually | |
| export CORE_ROOT=$(pwd)/artifacts/tests/coreclr/<os>.<arch>.Release/Tests/Core_Root | |
| cd artifacts/tests/coreclr/<os>.<arch>.Debug/readytorun/<TestName>/ | |
| src/tests/build.sh <buildType> -tree:src/tests/readytorun | |
| # Generate Core_Root layout (required for manual runs) | |
| src/tests/build.sh -GenerateLayoutOnly x64 <buildType> | |
| # Run a single test manually | |
| export CORE_ROOT=$(pwd)/artifacts/tests/coreclr/<os>.<arch>.<buildType>/Tests/Core_Root | |
| cd artifacts/tests/coreclr/<os>.<arch>.<buildType>/readytorun/<TestName>/ |
Summary
Improve Copilot instructions, skills, and documentation for ReadyToRun (R2R) test authoring based on lessons learned from PR #124770.
Changes
.github/copilot-instructions.md.github/skills/jit-regression-test/SKILL.md.github/skills/code-review/SKILL.mdsrc/tests/readytorun/README.md(new)Context
These improvements were developed during review of PR #124770 (R2R precompilation of blittable objc_msgSend P/Invokes). During that work, ~285 minutes were spent on issues that better documentation and review rules would have prevented, including incorrect DllImport paths, wrong map entry assertions, missing --inputbubble, and cross-component build failures.