Skip to content

Add DependencyAnalysisFramework tests from CoreRT repo#130849

Open
MichalStrehovsky wants to merge 2 commits into
dotnet:mainfrom
MichalStrehovsky:daftests
Open

Add DependencyAnalysisFramework tests from CoreRT repo#130849
MichalStrehovsky wants to merge 2 commits into
dotnet:mainfrom
MichalStrehovsky:daftests

Conversation

@MichalStrehovsky

Copy link
Copy Markdown
Member

We forgot these in the CoreRT repo. DependencyAnalysisFramework originally moved here with crossgen2, but without the testing. First commit just copies file over, second commit hooks them up to the build.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds the missing ILCompiler.DependencyAnalysisFramework unit test project to the runtime repo and wires it into the CoreCLR tools test subset so it participates in CI/tool test runs.

Changes:

  • Adds TestGraph test infrastructure for exercising static, conditional, and dynamic dependency marking.
  • Adds xUnit tests validating dependency marking behavior across logging strategies and validating DGML output shape.
  • Hooks the new test project into the +clr.toolstests+ subset build/test list.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework.Tests/TestGraph.cs Adds a small test graph/node model used by the unit tests to drive the dependency analyzer.
src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework.Tests/DependencyAnalysisFrameworkTests.cs Adds xUnit tests for dependency marking and DGML output across different log strategies.
src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework.Tests/ILCompiler.DependencyAnalysisFramework.Tests.csproj Introduces the new test project and references the DependencyAnalysisFramework project + xUnit extensions.
eng/Subsets.props Ensures the new test project runs as part of the clr.toolstests subset.

Comment on lines +6 to +8
using System.Linq;
using System.Text;
using System.Threading.Tasks;
Comment on lines +108 to +111
public void AddRoot(string nodeNode, string reason)
{
_analyzer.AddRoot(this.GetNode(nodeNode), reason);
}
Comment on lines +3 to +6
<AssemblyName>ILCompiler.DependencyAnalysisFramework.Tests</AssemblyName>
<TargetFramework>$(NetCoreAppToolCurrent)</TargetFramework>
<Configurations>Debug;Release;Checked</Configurations>
<TestRunnerAdditionalArguments>-notrait category=failing</TestRunnerAdditionalArguments>
@github-actions

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "c9a00a5d86a6f3fd87458fcf3b342dad6a34f4a6",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "ac90ce8e7a5c217d4ab9effb35c82e5b6596371f",
  "last_reviewed_commit": "c9a00a5d86a6f3fd87458fcf3b342dad6a34f4a6",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "ac90ce8e7a5c217d4ab9effb35c82e5b6596371f",
  "last_recorded_worker_run_id": "29684897252",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "c9a00a5d86a6f3fd87458fcf3b342dad6a34f4a6",
      "review_id": 4730668815
    }
  ]
}

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holistic Review

Motivation: The ILCompiler.DependencyAnalysisFramework component was moved into dotnet/runtime alongside crossgen2, but its unit tests were left behind in the old CoreRT repository. This PR restores that lost test coverage for the dependency-analysis graph marking logic (full/first-mark/no-log strategies, static/conditional/dynamic rules, and DGML output).

Approach: The change copies the original TestGraph.cs helper and DependencyAnalysisFrameworkTests.cs xUnit tests verbatim, adds a new ILCompiler.DependencyAnalysisFramework.Tests.csproj that references the framework project and Microsoft.DotNet.XUnitExtensions, and wires the project into the clr.toolstests subset in eng/Subsets.props (guarded by Condition="'$(DotNetBuildSourceOnly)' != 'true'", consistent with the neighboring AOT test projects). The split into a copy commit followed by a build-hookup commit keeps the port easy to audit.

Summary: This is a low-risk, test-only addition that recovers previously-existing coverage. The csproj mirrors the conventions of sibling AOT test projects (TargetFramework=$(NetCoreAppToolCurrent), Configurations, TestRunnerAdditionalArguments, NU1701 suppression). The ported code uses older C# idioms (explicit Tuple<>, new T[0], out-var-free TryGetValue) rather than the repository's current conventions, but that is expected and appropriate for a faithful port and not worth churning. No functional runtime code is affected. LGTM.

Detailed Findings

No actionable issues found. Two non-blocking observations, neither warranting a change:

  • TestGraph.cs and the test bodies retain legacy style (e.g. s_emptyDynamicList = new CombinedDependencyListEntry[0], explicit Tuple<> allocations, Assert.True(results.Count == N) instead of Assert.Equal(N, results.Count)). Preserving the original source verbatim is reasonable for a port; modernizing is optional.
  • Unlike some sibling AOT test csproj files, this project omits EnableDefaultItems=false/CopyLocalLockFileAssemblies, which is correct here since it has no test-asset subprojects or generated content to exclude.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 52.6 AIC · ⌖ 10.2 AIC · ⊞ 10K

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants