Skip to content

[cDAC] Refactor cDAC test structure: extract reusable harness, organize under tests/#128863

Open
max-charlamb wants to merge 2 commits into
dotnet:mainfrom
max-charlamb:dev/maxcharlamb/cdac-test-infrastructure-package
Open

[cDAC] Refactor cDAC test structure: extract reusable harness, organize under tests/#128863
max-charlamb wants to merge 2 commits into
dotnet:mainfrom
max-charlamb:dev/maxcharlamb/cdac-test-infrastructure-package

Conversation

@max-charlamb
Copy link
Copy Markdown
Member

@max-charlamb max-charlamb commented Jun 1, 2026

Note

This PR description was drafted with assistance from GitHub Copilot.

Reorganize the cDAC test layout so the in-repo unit tests and the reusable test-authoring infrastructure are clearly separated and discoverable.

Changes

Move unit-test project under tests/UnitTests/

The unit-test project lived directly at src/native/managed/cdac/tests/ while its sibling test projects (DumpTests, StressTests, DataGenerator) all use their own subfolder. This PR moves the unit tests into tests/UnitTests/ to match that convention, so tests/ becomes a clean parent of per-project subfolders.

Extract reusable test-harness into tests/TestInfrastructure/

The cDAC test projects contain a non-trivial body of reusable test-authoring building blocks:

  • MockMemorySpace and MockTarget infrastructure
  • ContractDescriptorBuilder / ContractDescriptorHelpers
  • ClrMdDumpHost, DumpTestBase, DumpTestStackWalker, DumpInfo, TestConfiguration
  • SkipOnArchAttribute / SkipOnOSAttribute / SkipOnVersionAttribute
  • TestHelpers / TargetTestHelpers / TestPlaceholderTarget
  • Layout / TypedView

These previously lived inside the Tests and DumpTests projects (with some files cross-linked via Compile Include). This PR factors them into a new library project:

src/native/managed/cdac/tests/TestInfrastructure/Microsoft.Diagnostics.DataContractReader.TestInfrastructure.csproj

It's a library, not a test project, so:

  • consumers can ProjectReference it without inheriting IsTestProject semantics or transitive xunit runner dependencies
  • it can be packed as a transport NuGet alongside the other cDAC transport packages (added to tools.cdac packing subset in eng/Subsets.props)
  • all harness types are exposed as public, so downstream test projects author against the harness with no InternalsVisibleTo grant required in either direction. The harness only references the public Abstractions / Contracts / DataContractReader surface — it does not require IVT into any cDAC assembly.

The in-repo Tests and DumpTests projects now ProjectReference TestInfrastructure instead of carrying these sources directly, which also lets DumpTests drop redundant transitive PackageReferences on Microsoft.Diagnostics.Runtime and Microsoft.DotNet.XUnitExtensions.

Final layout

src/native/managed/cdac/tests/
├── TestInfrastructure/   ← reusable harness library (packs as nupkg)
├── UnitTests/            ← test project
├── DumpTests/            ← test project
├── StressTests/
└── DataGenerator/

Verification

  • TestInfrastructure, Tests, and DumpTests all build clean (0 errors, 0 warnings)
  • 2429 unit tests pass / 16 skipped / 0 failed (unchanged from baseline)
  • dotnet pack on TestInfrastructure produces a clean nupkg in artifacts/packages/Release/NonShipping/

Notes for reviewers

The bulk of the diff is git mv renames. The material edits are limited to:

  • the test csproj files (Tests.csproj, DumpTests.csproj, new TestInfrastructure.csproj)
  • cdac.slnx and eng/Subsets.props (paths / new packing entry)
  • no new InternalsVisibleTo grants are added to any shipping cDAC assembly

The new TestInfrastructure.csproj enables <Nullable>enable</Nullable> and carries no <NoWarn> list, matching the analyzer posture of the other cDAC projects under src/native/managed/cdac/. The moved sources were already nullable-aware (only a handful of new CS86xx warnings appeared, vs. the ~200 CS8632 warnings eliminated by enabling nullable), and the remaining CA/SA/IDE rule violations are fixed in place.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR restructures the cDAC test tree by separating reusable test-authoring infrastructure into a dedicated TestInfrastructure library and relocating the unit-test project under tests/UnitTests/, updating solution/build subset wiring accordingly.

Changes:

  • Moved the unit test project/files into src/native/managed/cdac/tests/UnitTests/.
  • Extracted shared test harness code into src/native/managed/cdac/tests/TestInfrastructure/ as a packable helper library and updated test projects to reference it.
  • Updated build/solution documentation and subset definitions (cdac.slnx, eng/Subsets.props, cdac/README.md) to match the new layout.

Reviewed changes

Copilot reviewed 11 out of 88 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/native/managed/cdac/tests/UnitTests/TypeDescTests.cs Unit tests for RuntimeTypeSystem TypeDesc behaviors in new UnitTests location.
src/native/managed/cdac/tests/UnitTests/SyncBlockTests.cs Unit tests for SyncBlock contract behaviors.
src/native/managed/cdac/tests/UnitTests/SignatureTests.cs Unit tests for vararg signature/cookie behaviors.
src/native/managed/cdac/tests/UnitTests/RuntimeInfoTests.cs Unit tests for RuntimeInfo architecture/OS/version globals.
src/native/managed/cdac/tests/UnitTests/ReJITTests.cs Unit tests for ReJIT contract behavior using mocks/builders.
src/native/managed/cdac/tests/UnitTests/PlatformContextTests.cs Unit tests for per-arch register context helpers.
src/native/managed/cdac/tests/UnitTests/ObjectiveCMarshalTests.cs Unit tests for ObjectiveCMarshal contract behavior.
src/native/managed/cdac/tests/UnitTests/NotificationsTests.cs Unit tests for notification parsing.
src/native/managed/cdac/tests/UnitTests/LoaderHeapTests.cs Unit tests for LoaderHeap traversal/data.
src/native/managed/cdac/tests/UnitTests/GetRegisterNameTests.cs Unit tests for SOSDac register name mapping logic.
src/native/managed/cdac/tests/UnitTests/ExecutionManager/RuntimeFunctionTests.cs Unit tests for runtime function lookup/length computation.
src/native/managed/cdac/tests/UnitTests/ExecutionManager/RangeSectionMapTests.cs Unit tests for range section map lookup/indexing.
src/native/managed/cdac/tests/UnitTests/ExecutionManager/NibbleMapTests.cs Unit tests for nibble map helpers and lookup variants.
src/native/managed/cdac/tests/UnitTests/ExecutionManager/HotColdLookupTests.cs Unit tests for hot/cold lookup mapping behavior.
src/native/managed/cdac/tests/UnitTests/ExecutionManager/HashMapTests.cs Unit tests for hashmap/ptr-hashmap lookup behavior.
src/native/managed/cdac/tests/UnitTests/EnumMethodDefinitionsTests.cs Unit tests for method enumeration logic over synthetic metadata.
src/native/managed/cdac/tests/UnitTests/DacStreamsTests.cs Unit tests for dac stream decoding behaviors.
src/native/managed/cdac/tests/UnitTests/ContractDescriptor/TargetTests.SubDescriptors.cs Unit tests for contract descriptor sub-descriptor handling.
src/native/managed/cdac/tests/UnitTests/ClrDataTaskTests.cs Unit tests for ClrDataTask current app domain behavior.
src/native/managed/cdac/tests/UnitTests/AuxiliarySymbolsTests.cs Unit tests for auxiliary symbol name lookup behavior.
src/native/managed/cdac/tests/UnitTests/Microsoft.Diagnostics.DataContractReader.Tests.csproj Unit test project moved under UnitTests and updated to reference TestInfrastructure.
src/native/managed/cdac/tests/TestInfrastructure/TestPlaceholderTarget.cs Test harness target implementation updated (incl. ctor nullability annotation change).
src/native/managed/cdac/tests/TestInfrastructure/TestHelpers.cs New shared HResult formatting/assertion helper for tests.
src/native/managed/cdac/tests/TestInfrastructure/TestConfiguration.cs Moved dump test configuration type into shared infrastructure.
src/native/managed/cdac/tests/TestInfrastructure/TargetTestHelpers.cs Shared test helpers for layout + endian-aware reads/writes.
src/native/managed/cdac/tests/TestInfrastructure/SkipOnVersionAttribute.cs Moved dump-test skip attribute into shared infrastructure.
src/native/managed/cdac/tests/TestInfrastructure/SkipOnOSAttribute.cs Moved dump-test skip attribute into shared infrastructure.
src/native/managed/cdac/tests/TestInfrastructure/SkipOnArchAttribute.cs Moved dump-test skip attribute into shared infrastructure.
src/native/managed/cdac/tests/TestInfrastructure/README.md Documentation for the new TestInfrastructure library package.
src/native/managed/cdac/tests/TestInfrastructure/MockTarget.cs Shared mock target architecture enumeration support.
src/native/managed/cdac/tests/TestInfrastructure/MockMemorySpace.cs Shared mock memory space implementation.
src/native/managed/cdac/tests/TestInfrastructure/MockMemorySpace.BumpAllocator.cs Shared allocator for heap fragments (contains an allocation-size issue; see review comment).
src/native/managed/cdac/tests/TestInfrastructure/MockDescriptors/TypedView.cs Shared typed view base for mock descriptor structures.
src/native/managed/cdac/tests/TestInfrastructure/MockDescriptors/MockDescriptors.SyncBlock.cs Shared sync block mock descriptors/builders.
src/native/managed/cdac/tests/TestInfrastructure/MockDescriptors/MockDescriptors.RuntimeFunctions.cs Shared runtime function/unwind mock descriptors/builders.
src/native/managed/cdac/tests/TestInfrastructure/MockDescriptors/MockDescriptors.ReJIT.cs Shared ReJIT mock descriptors/builders.
src/native/managed/cdac/tests/TestInfrastructure/MockDescriptors/MockDescriptors.HashMap.cs Shared hash map mock descriptors/builders.
src/native/managed/cdac/tests/TestInfrastructure/MockDescriptors/MockDescriptors.Frame.cs Shared frame mock descriptors/builders.
src/native/managed/cdac/tests/TestInfrastructure/MockDescriptors/Layout.cs Shared layout builders used by mock descriptors.
src/native/managed/cdac/tests/TestInfrastructure/ExecutionManager/NibbleMapTestBuilder.cs Shared nibble map test builder moved into infrastructure.
src/native/managed/cdac/tests/TestInfrastructure/DumpTestHelpers.cs Shared dump test helper utilities.
src/native/managed/cdac/tests/TestInfrastructure/DumpInfo.cs Shared dump-info model + serialization helpers.
src/native/managed/cdac/tests/TestInfrastructure/ContractDescriptor/ContractDescriptorHelpers.cs Shared helpers for constructing contract descriptors in tests.
src/native/managed/cdac/tests/TestInfrastructure/ContractDescriptor/ContractDescriptorBuilder.cs Shared builder for synthetic contract descriptors/targets.
src/native/managed/cdac/tests/TestInfrastructure/ClrMdDumpHost.cs Shared ClrMD-based dump host implementation.
src/native/managed/cdac/tests/TestInfrastructure/Microsoft.Diagnostics.DataContractReader.TestInfrastructure.csproj New reusable harness library project (packable) with references/dependencies.
src/native/managed/cdac/tests/DumpTests/Microsoft.Diagnostics.DataContractReader.DumpTests.csproj Updated dump tests project to reference TestInfrastructure and drop redundant package refs/linking.
src/native/managed/cdac/tests/Microsoft.Diagnostics.DataContractReader.Tests.csproj Removed old unit test csproj at the former location.
src/native/managed/cdac/README.md Updated documentation to point at new UnitTests project path.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Microsoft.Diagnostics.DataContractReader.Legacy.csproj Added InternalsVisibleTo for TestInfrastructure.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Microsoft.Diagnostics.DataContractReader.Contracts.csproj Added InternalsVisibleTo for TestInfrastructure.
src/native/managed/cdac/cdac.slnx Updated solution to include TestInfrastructure + UnitTests projects in tests folder.
eng/Subsets.props Updated tools.cdac/tools.cdactests subset project lists for new project structure.

@max-charlamb max-charlamb force-pushed the dev/maxcharlamb/cdac-test-infrastructure-package branch from 6b0e1d8 to d4b5c7e Compare June 1, 2026 18:31
@max-charlamb max-charlamb changed the title Refactor cDAC test structure: extract reusable harness, organize under tests/ [cDAC] Refactor cDAC test structure: extract reusable harness, organize under tests/ Jun 1, 2026
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 22 out of 88 changed files in this pull request and generated 2 comments.

Comment thread src/native/managed/cdac/tests/TestInfrastructure/README.md Outdated
@max-charlamb max-charlamb force-pushed the dev/maxcharlamb/cdac-test-infrastructure-package branch from b466cbe to d6d5738 Compare June 2, 2026 19:49
Copilot AI review requested due to automatic review settings June 2, 2026 20:09
@max-charlamb max-charlamb force-pushed the dev/maxcharlamb/cdac-test-infrastructure-package branch from d6d5738 to 93bac61 Compare June 2, 2026 20:09
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 37 out of 87 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/native/managed/cdac/tests/TestInfrastructure/TypedView.cs:20

  • TypedView.Init is part of the internal initialization contract used by Layout<TView>.Create(...). Making it public unnecessarily expands the surface area and lets consumers put a TypedView into a partially-initialized state. Prefer protected internal so it remains callable from the infrastructure assembly without becoming part of the external API.

@max-charlamb max-charlamb force-pushed the dev/maxcharlamb/cdac-test-infrastructure-package branch from 93bac61 to 3cce099 Compare June 3, 2026 16:16
Copilot AI review requested due to automatic review settings June 3, 2026 16:50
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 42 out of 92 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/native/managed/cdac/tests/TestInfrastructure/TestPlaceholderTarget.cs:28

  • This change (and similar ones across TestInfrastructure) introduces new public API surface (e.g., making TestPlaceholderTarget/MockMemorySpace/TargetTestHelpers/Layout/TypedView public and adding a new packable TestInfrastructure library). In dotnet/runtime, new public APIs require an approved API proposal (linked issue with the api-approved label, with the approved shape), or the APIs should remain internal pending review. The PR description doesn’t reference any api-approved issue, so this is currently unreviewed public surface.

…r tests/

Reorganize the cdac test layout so the in-repo unit tests and the reusable
test-authoring infrastructure are clearly separated and discoverable.

Changes:

* Move the unit-test project from tests/ into tests/UnitTests/ to match the
  existing tests/DumpTests/ convention. Sibling tests/StressTests/ and
  tests/DataGenerator/ already follow this pattern.

* Extract the test-harness building blocks (mock-memory infrastructure,
  ContractDescriptorBuilder, ClrMD-based dump host, DumpTestBase +
  DumpTestStackWalker, Skip* attributes, TargetTestHelpers, Layout/TypedView,
  TestPlaceholderTarget, etc.) into a new library project
  tests/TestInfrastructure/Microsoft.Diagnostics.DataContractReader.TestInfrastructure.csproj.

  This is a library (not a test project), so consumers can write their own
  xUnit test projects that ProjectReference the harness without picking up
  IsTestProject semantics or transitive xunit.runner dependencies. The
  package is added to the tools.cdac packing subset and ships as a transport
  NuGet alongside the other cdac transport packages.

  All harness types are public, so consumers do not need any IVT grant in
  either direction. The harness references only the public Abstractions /
  Contracts / DataContractReader surface and HResults is Compile-included
  directly (matching the existing pattern in DataContractReader.csproj,
  Legacy.csproj, and mscordaccore_universal.csproj).

  The in-repo Tests and DumpTests projects now ProjectReference
  TestInfrastructure instead of carrying the harness sources directly.

* Add DumpTestStackWalker.IsLegacyVisible (public) and
  DumpTestStackWalker.LegacyVisibleFrames(IStackWalk, ThreadData) so dump
  tests no longer depend on the internal ClrDataStackWalk.IsLegacyVisible.
  All call sites switch to the helper.

* Update cdac.slnx, eng/Subsets.props, and src/native/managed/cdac/README.md
  for the new paths.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@max-charlamb max-charlamb force-pushed the dev/maxcharlamb/cdac-test-infrastructure-package branch from dfbb7c1 to eb9cec4 Compare June 3, 2026 17:01
Copilot AI review requested due to automatic review settings June 3, 2026 17:38
@max-charlamb max-charlamb force-pushed the dev/maxcharlamb/cdac-test-infrastructure-package branch from 4847510 to 47482b4 Compare June 3, 2026 17:41
@max-charlamb max-charlamb force-pushed the dev/maxcharlamb/cdac-test-infrastructure-package branch from 47482b4 to bf99593 Compare June 3, 2026 17:43
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 121 out of 126 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src/native/managed/cdac/tests/TestInfrastructure/TypedView.cs:10

  • This change makes TypedView public as part of a new packable library project. That introduces new public API surface area; dotnet/runtime policy typically requires an approved API proposal (linked api-approved issue) for new public APIs, or keeping the APIs internal until review. The PR description doesn't link an approved API issue, so it's currently unclear how this public surface is being tracked/approved.
    src/native/managed/cdac/tests/UnitTests/ExecutionManager/RangeSectionMapTests.cs:58
  • The mask for irrelevantLowBits looks off by one: the comment says bits 0..effectiveRange-1 are irrelevant (i.e., effectiveRange bits), but the mask uses 1ul << (effectiveRange - 1), which only covers effectiveRange-1 bits. This reduces coverage for the lowest irrelevant bit and may not exercise the intended address patterns.
    src/native/managed/cdac/tests/UnitTests/ExecutionManager/NibbleMapTests.cs:43
  • Test method name and comment contain typos (ExhaustiveNibbbleShifts, expct). Since this is new code, it’s worth fixing to keep the test suite easy to search and read.

@max-charlamb max-charlamb force-pushed the dev/maxcharlamb/cdac-test-infrastructure-package branch 2 times, most recently from 7a6b93a to f863b0e Compare June 3, 2026 18:43
Copilot AI review requested due to automatic review settings June 3, 2026 18:43
The harness types in tests/TestInfrastructure/ now live in the
Microsoft.Diagnostics.DataContractReader.TestInfrastructure namespace
(and TestInfrastructure.ContractDescriptor for the ContractDescriptor
subfolder), instead of the previous mix of
Microsoft.Diagnostics.DataContractReader.Tests and
Microsoft.Diagnostics.DataContractReader.DumpTests namespaces. This
matches the project/folder name and makes the harness's identity
obvious at consumption sites.

Add GlobalUsings.cs to the in-repo UnitTests and DumpTests projects
that global-using the new namespace(s), so existing test files don't
need per-file using directives.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@max-charlamb max-charlamb force-pushed the dev/maxcharlamb/cdac-test-infrastructure-package branch from f863b0e to 991072f Compare June 3, 2026 18:45
@max-charlamb max-charlamb marked this pull request as ready for review June 3, 2026 18:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 121 out of 126 changed files in this pull request and generated no new comments.

@max-charlamb max-charlamb enabled auto-merge (squash) June 3, 2026 20:42
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.

4 participants