-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Add end-to-end round-trip tests for System.Reflection.Metadata resources #123578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…etadata resources Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
…leaks Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="$(MicrosoftCodeAnalysisVersion_LatestVS)" PrivateAssets="all" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Avoid dependency on Microsoft.CodeAnalysis.CSharp in these tests. Microsoft.CodeAnalysis.CSharp is very heavy weight dependency and it would prevent this whole test assembly from being able to run on devices, wasm and nativeaot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Removed the Microsoft.CodeAnalysis.CSharp dependency and rewrote the tests to use MetadataBuilder and BlobBuilder directly. The tests now manually construct assembly metadata without Roslyn, following the same patterns as PEBuilderTests.cs. All 3 tests pass successfully. Changes in commit 68d5962.
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
@agocke General assembly round tripper requires deserializing the assembly into an object model, and then serializing the object model again. I think direct rewriting without some intermediate representation would be very messy (you have to deal with cycles, etc.) We have MetadataLoadContext/PersistedAssemblyBuilder in this repo. We have built a roundtrip tool for testing PersistedAssemblyBuilder, unfortunately we did not have time to clean it up and get it committed into the repo. We are working on rewriting ilasm/ildasm on top of System.Reflection.Metadata. It is also going to be a great practical test case for roundtripping. We may want to spend energy on one of these instead of trying to create yet another roundtripping object model just for test purposes. |
|
Tagging subscribers to this area: @dotnet/area-system-reflection-metadata |
Description
System.Reflection.Metadata lacked comprehensive tests validating that assemblies with resources can be read via PEReader/MetadataReader and rebuilt via MetadataBuilder/ManagedPEBuilder without data loss. Added three test cases covering the risk surface: embedded resources, managed .resources payloads, and mixed scenarios.
Changes
Test Infrastructure
RoundTripTests.csinPortableExecutable/directoryTest Coverage
RoundTrip_Assembly_WithEmbeddedResources: Validates embedded manifest resources with varying sizes (100B, 1KB, empty), Unicode names (Cyrillic, Japanese, emoji), and byte-exact preservationRoundTrip_Assembly_WithManagedResourcesPayload: Validates System.Resources format (.resources) with multiple data types, verifying ResourceReader can enumerate entries post-roundtripRoundTrip_Assembly_WithMixedResources: Validates combined embedded + managed resources in single assemblyImplementation
PEBuilderTests.cspatternsTesting
Full test suite: 878 passed, 1 skipped (879 total)
Original prompt
This pull request was created from Copilot chat.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.