Skip to content

Refine PersistedAssemblyBuilder test coverage per review feedback, helper deduplication, and AssemblyNameInfo-based identity validation#124626

Merged
AaronRobinsonMSFT merged 10 commits intomainfrom
copilot/review-missing-test-scenarios-again
Feb 22, 2026
Merged

Refine PersistedAssemblyBuilder test coverage per review feedback, helper deduplication, and AssemblyNameInfo-based identity validation#124626
AaronRobinsonMSFT merged 10 commits intomainfrom
copilot/review-missing-test-scenarios-again

Conversation

Copy link
Contributor

Copilot AI commented Feb 20, 2026 •
edited
Loading

This PR reflects the final review-driven scope for PersistedAssemblyBuilder coverage and follow-up cleanup requested during review.

Final scope after feedback

Based on PR review comments, tests considered out-of-scope or not uniquely exercising PersistedAssemblyBuilder were removed, relevant validation was consolidated into existing tests, duplicate helper logic was removed in favor of existing shared helpers, and assembly identity validation was simplified to use AssemblyNameInfo.

What is included

Updated focused tests in:

  • /src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveAssemblyBuilderTests.cs
  • /src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveILGeneratorTests.cs
  • /src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveCustomAttributeTests.cs

Coverage retained in final PR:

  1. Assembly identity edge cases

    • Validates persisted identity by converting metadata via assemblyDefinition.GetAssemblyNameInfo().ToAssemblyName().
    • Checks exact roundtrip of Name, Version, CultureName, Flags, and PublicKey.
    • Keeps ContentType validation using AssemblyName properties (without hardcoded encoding-shift constants).

  2. Save/Generate lifecycle interactions

    • Folded into existing validation tests:
      • PersistedAssemblyBuilder_SaveValidations
      • PersistedAssemblyBuilder_GenerateMetadataValidation

  3. Cross-assembly nested generic references with constraints

    • Keeps cross-assembly generic reference scenario with corrected type-name assertion behavior.

  4. Attribute edge cases (named args + enum arrays)

    • Keeps named-argument-only custom attribute round-trip coverage for enum-array and enum-value payloads.

Additional review-driven cleanup

  • Removed the local CreateSimplePersistedAssembly helper from AssemblySaveAssemblyBuilderTests.
  • Reused existing helper patterns (notably PopulateAssemblyBuilderAndTypeBuilder) where applicable, with inline setup only where custom assembly naming is required.
  • Simplified identity extraction per feedback by using GetAssemblyNameInfo().ToAssemblyName() directly.
  • Added unique culture roundtrip coverage (tr-TR) in the identity test.

What was removed during review

Per reviewer sentiment and specific comments, removed tests that were not considered strong PersistedAssemblyBuilder coverage in this PR:

  • AssemblyLoadContext isolation-focused test
  • Metadata invariants/determinism tests
  • Stream corner-case handling tests
  • Additional entry-point-shape tests beyond existing entry-point coverage

Cleanup

  • Removed /src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/MissingScenarios.txt.

Validation

  • Ran targeted System.Reflection.Emit tests for the retained scenario set:
    • Passed: 6
    • Failed: 0
    • Skipped: 0
  • Re-ran targeted identity/validation tests after AssemblyNameInfo simplification:
    • Passed: 3
    • Failed: 0
    • Skipped: 0
  • Ran targeted identity roundtrip test after unique-culture update:
    • Passed: 1
    • Failed: 0
    • Skipped: 0

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

Copilot AI changed the title [WIP] Review .NET reflection stack for missing test scenarios Add PersistedAssemblyBuilder test-gap inventory for reflection coverage planning Feb 20, 2026
@dotnet-policy-service
Copy link
Contributor

dotnet-policy-service bot commented Feb 20, 2026

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

@build-analysis build-analysis bot mentioned this pull request Feb 20, 2026
Open
@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Feb 20, 2026

@copilot Please address items 1, 2, 3 and 5 in the list.

Copy link
Contributor Author

Copilot AI commented Feb 20, 2026

@copilot Please address items 1, 2, 3 and 5 in the list.

Addressed in 0227991. I added focused PersistedAssemblyBuilder tests for items 1, 2, 3, and 5 (assembly identity flags/public key + processor architecture encoding, save/generate lifecycle behavior, Save(Stream) edge cases, and nested/generic entry point scenarios), and validated with targeted System.Reflection.Emit tests.

Copilot AI changed the title Add PersistedAssemblyBuilder test-gap inventory for reflection coverage planning Add PersistedAssemblyBuilder coverage tests for requested missing scenarios Feb 20, 2026
@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Feb 20, 2026

@copilot Please now address 6, 7, 8 and 9. Once those are also complete, please remove the txt file.

Copy link
Contributor Author

Copilot AI commented Feb 20, 2026

@copilot Please now address 6, 7, 8 and 9. Once those are also complete, please remove the txt file.

Done in ceaeff5. I added tests for items 6, 7, 8, and 9 (collectible ALC isolation, metadata invariants for equivalent assemblies, cross-assembly nested generic reference with constraint, and named-argument enum-array custom attribute round-trip), and removed MissingScenarios.txt.

Copilot AI changed the title Add PersistedAssemblyBuilder coverage tests for requested missing scenarios Add PersistedAssemblyBuilder coverage tests for requested scenarios and remove gap list Feb 20, 2026
@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as ready for review February 20, 2026 22:49
Copilot AI review requested due to automatic review settings February 20, 2026 22:49
Copilot AI reviewed Feb 20, 2026
View reviewed changes
Copy link
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

Adds additional executable test coverage for PersistedAssemblyBuilder persistence scenarios in System.Reflection.Emit tests, aimed at closing previously identified gaps (identity encoding, lifecycle validation, stream edge cases, entry point shapes, ALC isolation, metadata invariants, and attribute payload edge cases).

Changes:

  • Added new PersistedAssemblyBuilder tests for assembly identity flag encoding, Save/GenerateMetadata lifecycle rules, and Save(Stream) edge behaviors.
  • Added tests for nested/generic entry points, collectible AssemblyLoadContext isolation, and basic metadata determinism/invariants.
  • Added tests for cross-assembly generic type references and for named-argument-only custom attributes with enum-array payloads.

Reviewed changes

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

File Description
src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveILGeneratorTests.cs Adds a cross-assembly generic reference test involving constrained generic parameters.
src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveCustomAttributeTests.cs Adds a round-trip test for named-only custom attributes using enum and enum-array named arguments (plus helper types).
src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveAssemblyBuilderTests.cs Adds multiple coverage tests for identity flag encoding, lifecycle validation, stream behavior, entry point handling, ALC isolation, and metadata invariants (plus small helpers).

Copilot AI changed the title Refine PersistedAssemblyBuilder test coverage per review feedback Refine PersistedAssemblyBuilder test coverage per review feedback and helper deduplication Feb 21, 2026
Copilot AI requested a review from jkotas February 21, 2026 15:58
Copilot AI changed the title Refine PersistedAssemblyBuilder test coverage per review feedback and helper deduplication Refine PersistedAssemblyBuilder test coverage per review feedback, helper deduplication, and AssemblyName-based identity validation Feb 21, 2026
Copilot AI changed the title Refine PersistedAssemblyBuilder test coverage per review feedback, helper deduplication, and AssemblyName-based identity validation Refine PersistedAssemblyBuilder test coverage per review feedback, helper deduplication, and AssemblyNameInfo-based identity validation Feb 22, 2026
Copilot AI requested a review from jkotas February 22, 2026 02:29
Copilot AI requested a review from jkotas February 22, 2026 03:49
@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 2fbf43f into main Feb 22, 2026
91 of 93 checks passed
@github-project-automation github-project-automation bot moved this to Done in AppModel Feb 22, 2026
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the copilot/review-missing-test-scenarios-again branch February 22, 2026 18:18
This was referenced Feb 23, 2026
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@jkotas jkotas jkotas approved these changes

+1 more reviewer

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@AaronRobinsonMSFT AaronRobinsonMSFT

Copilot code review Copilot

Labels

area-System.Reflection.Emit

Projects

AppModel
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AaronRobinsonMSFT @jkotas